diff mbox series

[v9,19/19] x86: EFI stub DRTM launch support for Secure Launch

Message ID 20240531010331.134441-20-ross.philipson@oracle.com
State New
Headers show
Series x86: Trenchboot secure dynamic launch Linux kernel support | expand

Commit Message

Ross Philipson May 31, 2024, 1:03 a.m. UTC
This support allows the DRTM launch to be initiated after an EFI stub
launch of the Linux kernel is done. This is accomplished by providing
a handler to jump to when a Secure Launch is in progress. This has to be
called after the EFI stub does Exit Boot Services.

Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
---
 drivers/firmware/efi/libstub/x86-stub.c | 98 +++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

Comments

Ard Biesheuvel May 31, 2024, 11:09 a.m. UTC | #1
On Fri, 31 May 2024 at 03:32, Ross Philipson <ross.philipson@oracle.com> wrote:
>
> This support allows the DRTM launch to be initiated after an EFI stub
> launch of the Linux kernel is done. This is accomplished by providing
> a handler to jump to when a Secure Launch is in progress. This has to be
> called after the EFI stub does Exit Boot Services.
>
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>

Just some minor remarks below. The overall approach in this patch
looks fine now.


> ---
>  drivers/firmware/efi/libstub/x86-stub.c | 98 +++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index d5a8182cf2e1..a1143d006202 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -9,6 +9,8 @@
>  #include <linux/efi.h>
>  #include <linux/pci.h>
>  #include <linux/stddef.h>
> +#include <linux/slr_table.h>
> +#include <linux/slaunch.h>
>
>  #include <asm/efi.h>
>  #include <asm/e820/types.h>
> @@ -830,6 +832,97 @@ static efi_status_t efi_decompress_kernel(unsigned long *kernel_entry)
>         return efi_adjust_memory_range_protection(addr, kernel_text_size);
>  }
>
> +#if (IS_ENABLED(CONFIG_SECURE_LAUNCH))

IS_ENABLED() is mostly used for C conditionals not CPP ones.

It would be nice if this #if could be dropped, and replaced with ... (see below)


> +static bool efi_secure_launch_update_boot_params(struct slr_table *slrt,
> +                                                struct boot_params *boot_params)
> +{
> +       struct slr_entry_intel_info *txt_info;
> +       struct slr_entry_policy *policy;
> +       struct txt_os_mle_data *os_mle;
> +       bool updated = false;
> +       int i;
> +
> +       txt_info = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_INTEL_INFO);
> +       if (!txt_info)
> +               return false;
> +
> +       os_mle = txt_os_mle_data_start((void *)txt_info->txt_heap);
> +       if (!os_mle)
> +               return false;
> +
> +       os_mle->boot_params_addr = (u32)(u64)boot_params;
> +

Why is this safe?

> +       policy = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_ENTRY_POLICY);
> +       if (!policy)
> +               return false;
> +
> +       for (i = 0; i < policy->nr_entries; i++) {
> +               if (policy->policy_entries[i].entity_type == SLR_ET_BOOT_PARAMS) {
> +                       policy->policy_entries[i].entity = (u64)boot_params;
> +                       updated = true;
> +                       break;
> +               }
> +       }
> +
> +       /*
> +        * If this is a PE entry into EFI stub the mocked up boot params will
> +        * be missing some of the setup header data needed for the second stage
> +        * of the Secure Launch boot.
> +        */
> +       if (image) {
> +               struct setup_header *hdr = (struct setup_header *)((u8 *)image->image_base + 0x1f1);

Could we use something other than a bare 0x1f1 constant here? struct
boot_params has a struct setup_header at the correct offset, so with
some casting of offsetof() use, we can make this look a lot more self
explanatory.


> +               u64 cmdline_ptr, hi_val;
> +
> +               boot_params->hdr.setup_sects = hdr->setup_sects;
> +               boot_params->hdr.syssize = hdr->syssize;
> +               boot_params->hdr.version = hdr->version;
> +               boot_params->hdr.loadflags = hdr->loadflags;
> +               boot_params->hdr.kernel_alignment = hdr->kernel_alignment;
> +               boot_params->hdr.min_alignment = hdr->min_alignment;
> +               boot_params->hdr.xloadflags = hdr->xloadflags;
> +               boot_params->hdr.init_size = hdr->init_size;
> +               boot_params->hdr.kernel_info_offset = hdr->kernel_info_offset;
> +               hi_val = boot_params->ext_cmd_line_ptr;

We have efi_set_u64_split() for this.

> +               cmdline_ptr = boot_params->hdr.cmd_line_ptr | hi_val << 32;
> +               boot_params->hdr.cmdline_size = strlen((const char *)cmdline_ptr);;
> +       }
> +
> +       return updated;
> +}
> +
> +static void efi_secure_launch(struct boot_params *boot_params)
> +{
> +       struct slr_entry_dl_info *dlinfo;
> +       efi_guid_t guid = SLR_TABLE_GUID;
> +       dl_handler_func handler_callback;
> +       struct slr_table *slrt;
> +

... a C conditional here, e.g.,

if (!IS_ENABLED(CONFIG_SECURE_LAUNCH))
    return;

The difference is that all the code will get compile test coverage
every time, instead of only in configs that enable
CONFIG_SECURE_LAUNCH.

This significantly reduces the risk that your stuff will get broken
inadvertently.

> +       /*
> +        * The presence of this table indicated a Secure Launch
> +        * is being requested.
> +        */
> +       slrt = (struct slr_table *)get_efi_config_table(guid);
> +       if (!slrt || slrt->magic != SLR_TABLE_MAGIC)
> +               return;
> +
> +       /*
> +        * Since the EFI stub library creates its own boot_params on entry, the
> +        * SLRT and TXT heap have to be updated with this version.
> +        */
> +       if (!efi_secure_launch_update_boot_params(slrt, boot_params))
> +               return;
> +
> +       /* Jump through DL stub to initiate Secure Launch */
> +       dlinfo = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_DL_INFO);
> +
> +       handler_callback = (dl_handler_func)dlinfo->dl_handler;
> +
> +       handler_callback(&dlinfo->bl_context);
> +
> +       unreachable();
> +}
> +#endif
> +
>  static void __noreturn enter_kernel(unsigned long kernel_addr,
>                                     struct boot_params *boot_params)
>  {
> @@ -957,6 +1050,11 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>                 goto fail;
>         }
>
> +#if (IS_ENABLED(CONFIG_SECURE_LAUNCH))

... and drop this #if as well.

> +       /* If a Secure Launch is in progress, this never returns */
> +       efi_secure_launch(boot_params);
> +#endif
> +
>         /*
>          * Call the SEV init code while still running with the firmware's
>          * GDT/IDT, so #VC exceptions will be handled by EFI.
> --
> 2.39.3
>
Ross Philipson June 4, 2024, 5:22 p.m. UTC | #2
On 5/31/24 4:09 AM, Ard Biesheuvel wrote:
> On Fri, 31 May 2024 at 03:32, Ross Philipson <ross.philipson@oracle.com> wrote:
>>
>> This support allows the DRTM launch to be initiated after an EFI stub
>> launch of the Linux kernel is done. This is accomplished by providing
>> a handler to jump to when a Secure Launch is in progress. This has to be
>> called after the EFI stub does Exit Boot Services.
>>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> 
> Just some minor remarks below. The overall approach in this patch
> looks fine now.
> 
> 
>> ---
>>   drivers/firmware/efi/libstub/x86-stub.c | 98 +++++++++++++++++++++++++
>>   1 file changed, 98 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>> index d5a8182cf2e1..a1143d006202 100644
>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>> @@ -9,6 +9,8 @@
>>   #include <linux/efi.h>
>>   #include <linux/pci.h>
>>   #include <linux/stddef.h>
>> +#include <linux/slr_table.h>
>> +#include <linux/slaunch.h>
>>
>>   #include <asm/efi.h>
>>   #include <asm/e820/types.h>
>> @@ -830,6 +832,97 @@ static efi_status_t efi_decompress_kernel(unsigned long *kernel_entry)
>>          return efi_adjust_memory_range_protection(addr, kernel_text_size);
>>   }
>>
>> +#if (IS_ENABLED(CONFIG_SECURE_LAUNCH))
> 
> IS_ENABLED() is mostly used for C conditionals not CPP ones.
> 
> It would be nice if this #if could be dropped, and replaced with ... (see below)
> 
> 
>> +static bool efi_secure_launch_update_boot_params(struct slr_table *slrt,
>> +                                                struct boot_params *boot_params)
>> +{
>> +       struct slr_entry_intel_info *txt_info;
>> +       struct slr_entry_policy *policy;
>> +       struct txt_os_mle_data *os_mle;
>> +       bool updated = false;
>> +       int i;
>> +
>> +       txt_info = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_INTEL_INFO);
>> +       if (!txt_info)
>> +               return false;
>> +
>> +       os_mle = txt_os_mle_data_start((void *)txt_info->txt_heap);
>> +       if (!os_mle)
>> +               return false;
>> +
>> +       os_mle->boot_params_addr = (u32)(u64)boot_params;
>> +
> 
> Why is this safe?

The size of the boot_params_addr is a holdover from the legacy boot 
world when boot params were always loaded at a low address. We will 
increase the size of the field.

> 
>> +       policy = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_ENTRY_POLICY);
>> +       if (!policy)
>> +               return false;
>> +
>> +       for (i = 0; i < policy->nr_entries; i++) {
>> +               if (policy->policy_entries[i].entity_type == SLR_ET_BOOT_PARAMS) {
>> +                       policy->policy_entries[i].entity = (u64)boot_params;
>> +                       updated = true;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       /*
>> +        * If this is a PE entry into EFI stub the mocked up boot params will
>> +        * be missing some of the setup header data needed for the second stage
>> +        * of the Secure Launch boot.
>> +        */
>> +       if (image) {
>> +               struct setup_header *hdr = (struct setup_header *)((u8 *)image->image_base + 0x1f1);
> 
> Could we use something other than a bare 0x1f1 constant here? struct
> boot_params has a struct setup_header at the correct offset, so with
> some casting of offsetof() use, we can make this look a lot more self
> explanatory.

Yes we can do this.

> 
> 
>> +               u64 cmdline_ptr, hi_val;
>> +
>> +               boot_params->hdr.setup_sects = hdr->setup_sects;
>> +               boot_params->hdr.syssize = hdr->syssize;
>> +               boot_params->hdr.version = hdr->version;
>> +               boot_params->hdr.loadflags = hdr->loadflags;
>> +               boot_params->hdr.kernel_alignment = hdr->kernel_alignment;
>> +               boot_params->hdr.min_alignment = hdr->min_alignment;
>> +               boot_params->hdr.xloadflags = hdr->xloadflags;
>> +               boot_params->hdr.init_size = hdr->init_size;
>> +               boot_params->hdr.kernel_info_offset = hdr->kernel_info_offset;
>> +               hi_val = boot_params->ext_cmd_line_ptr;
> 
> We have efi_set_u64_split() for this.

Ok I will use that then.

> 
>> +               cmdline_ptr = boot_params->hdr.cmd_line_ptr | hi_val << 32;
>> +               boot_params->hdr.cmdline_size = strlen((const char *)cmdline_ptr);;
>> +       }
>> +
>> +       return updated;
>> +}
>> +
>> +static void efi_secure_launch(struct boot_params *boot_params)
>> +{
>> +       struct slr_entry_dl_info *dlinfo;
>> +       efi_guid_t guid = SLR_TABLE_GUID;
>> +       dl_handler_func handler_callback;
>> +       struct slr_table *slrt;
>> +
> 
> ... a C conditional here, e.g.,
> 
> if (!IS_ENABLED(CONFIG_SECURE_LAUNCH))
>      return;
> 
> The difference is that all the code will get compile test coverage
> every time, instead of only in configs that enable
> CONFIG_SECURE_LAUNCH.
> 
> This significantly reduces the risk that your stuff will get broken
> inadvertently.

Understood, I will address these as you suggest.

> 
>> +       /*
>> +        * The presence of this table indicated a Secure Launch
>> +        * is being requested.
>> +        */
>> +       slrt = (struct slr_table *)get_efi_config_table(guid);
>> +       if (!slrt || slrt->magic != SLR_TABLE_MAGIC)
>> +               return;
>> +
>> +       /*
>> +        * Since the EFI stub library creates its own boot_params on entry, the
>> +        * SLRT and TXT heap have to be updated with this version.
>> +        */
>> +       if (!efi_secure_launch_update_boot_params(slrt, boot_params))
>> +               return;
>> +
>> +       /* Jump through DL stub to initiate Secure Launch */
>> +       dlinfo = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_DL_INFO);
>> +
>> +       handler_callback = (dl_handler_func)dlinfo->dl_handler;
>> +
>> +       handler_callback(&dlinfo->bl_context);
>> +
>> +       unreachable();
>> +}
>> +#endif
>> +
>>   static void __noreturn enter_kernel(unsigned long kernel_addr,
>>                                      struct boot_params *boot_params)
>>   {
>> @@ -957,6 +1050,11 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>>                  goto fail;
>>          }
>>
>> +#if (IS_ENABLED(CONFIG_SECURE_LAUNCH))
> 
> ... and drop this #if as well.

Yes.

Thanks
Ross

> 
>> +       /* If a Secure Launch is in progress, this never returns */
>> +       efi_secure_launch(boot_params);
>> +#endif
>> +
>>          /*
>>           * Call the SEV init code while still running with the firmware's
>>           * GDT/IDT, so #VC exceptions will be handled by EFI.
>> --
>> 2.39.3
>>
diff mbox series

Patch

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index d5a8182cf2e1..a1143d006202 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -9,6 +9,8 @@ 
 #include <linux/efi.h>
 #include <linux/pci.h>
 #include <linux/stddef.h>
+#include <linux/slr_table.h>
+#include <linux/slaunch.h>
 
 #include <asm/efi.h>
 #include <asm/e820/types.h>
@@ -830,6 +832,97 @@  static efi_status_t efi_decompress_kernel(unsigned long *kernel_entry)
 	return efi_adjust_memory_range_protection(addr, kernel_text_size);
 }
 
+#if (IS_ENABLED(CONFIG_SECURE_LAUNCH))
+static bool efi_secure_launch_update_boot_params(struct slr_table *slrt,
+						 struct boot_params *boot_params)
+{
+	struct slr_entry_intel_info *txt_info;
+	struct slr_entry_policy *policy;
+	struct txt_os_mle_data *os_mle;
+	bool updated = false;
+	int i;
+
+	txt_info = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_INTEL_INFO);
+	if (!txt_info)
+		return false;
+
+	os_mle = txt_os_mle_data_start((void *)txt_info->txt_heap);
+	if (!os_mle)
+		return false;
+
+	os_mle->boot_params_addr = (u32)(u64)boot_params;
+
+	policy = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_ENTRY_POLICY);
+	if (!policy)
+		return false;
+
+	for (i = 0; i < policy->nr_entries; i++) {
+		if (policy->policy_entries[i].entity_type == SLR_ET_BOOT_PARAMS) {
+			policy->policy_entries[i].entity = (u64)boot_params;
+			updated = true;
+			break;
+		}
+	}
+
+	/*
+	 * If this is a PE entry into EFI stub the mocked up boot params will
+	 * be missing some of the setup header data needed for the second stage
+	 * of the Secure Launch boot.
+	 */
+	if (image) {
+		struct setup_header *hdr = (struct setup_header *)((u8 *)image->image_base + 0x1f1);
+		u64 cmdline_ptr, hi_val;
+
+		boot_params->hdr.setup_sects = hdr->setup_sects;
+		boot_params->hdr.syssize = hdr->syssize;
+		boot_params->hdr.version = hdr->version;
+		boot_params->hdr.loadflags = hdr->loadflags;
+		boot_params->hdr.kernel_alignment = hdr->kernel_alignment;
+		boot_params->hdr.min_alignment = hdr->min_alignment;
+		boot_params->hdr.xloadflags = hdr->xloadflags;
+		boot_params->hdr.init_size = hdr->init_size;
+		boot_params->hdr.kernel_info_offset = hdr->kernel_info_offset;
+		hi_val = boot_params->ext_cmd_line_ptr;
+		cmdline_ptr = boot_params->hdr.cmd_line_ptr | hi_val << 32;
+		boot_params->hdr.cmdline_size = strlen((const char *)cmdline_ptr);;
+	}
+
+	return updated;
+}
+
+static void efi_secure_launch(struct boot_params *boot_params)
+{
+	struct slr_entry_dl_info *dlinfo;
+	efi_guid_t guid = SLR_TABLE_GUID;
+	dl_handler_func handler_callback;
+	struct slr_table *slrt;
+
+	/*
+	 * The presence of this table indicated a Secure Launch
+	 * is being requested.
+	 */
+	slrt = (struct slr_table *)get_efi_config_table(guid);
+	if (!slrt || slrt->magic != SLR_TABLE_MAGIC)
+		return;
+
+	/*
+	 * Since the EFI stub library creates its own boot_params on entry, the
+	 * SLRT and TXT heap have to be updated with this version.
+	 */
+	if (!efi_secure_launch_update_boot_params(slrt, boot_params))
+		return;
+
+	/* Jump through DL stub to initiate Secure Launch */
+	dlinfo = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_DL_INFO);
+
+	handler_callback = (dl_handler_func)dlinfo->dl_handler;
+
+	handler_callback(&dlinfo->bl_context);
+
+	unreachable();
+}
+#endif
+
 static void __noreturn enter_kernel(unsigned long kernel_addr,
 				    struct boot_params *boot_params)
 {
@@ -957,6 +1050,11 @@  void __noreturn efi_stub_entry(efi_handle_t handle,
 		goto fail;
 	}
 
+#if (IS_ENABLED(CONFIG_SECURE_LAUNCH))
+	/* If a Secure Launch is in progress, this never returns */
+	efi_secure_launch(boot_params);
+#endif
+
 	/*
 	 * Call the SEV init code while still running with the firmware's
 	 * GDT/IDT, so #VC exceptions will be handled by EFI.