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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/31/24 4:09 AM, Ard Biesheuvel wrote:
On Fri, 31 May 2024 at 03:32, Ross Philipson <ross.philipson@xxxxxxxxxx> 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@xxxxxxxxxx>

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






[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux