| Message ID | 20240531010331.134441-20-ross.philipson@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | x86: Trenchboot secure dynamic launch Linux kernel support | expand |
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 >
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 --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.
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(+)