lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <C98F883A-31D5-4F67-97FF-4AEFAADDDC74@zytor.com>
Date: Wed, 21 Feb 2024 12:37:25 -0800
From: "H. Peter Anvin" <hpa@...or.com>
To: ross.philipson@...cle.com, Ard Biesheuvel <ardb@...nel.org>
CC: linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-integrity@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-crypto@...r.kernel.org, kexec@...ts.infradead.org,
        linux-efi@...r.kernel.org, dpsmith@...rtussolutions.com,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, mjg59@...f.ucam.org,
        James.Bottomley@...senpartnership.com, peterhuewe@....de,
        jarkko@...nel.org, jgg@...pe.ca, luto@...capital.net,
        nivedita@...m.mit.edu, herbert@...dor.apana.org.au,
        davem@...emloft.net, kanth.ghatraju@...cle.com,
        trenchboot-devel@...glegroups.com
Subject: Re: [PATCH v8 15/15] x86: EFI stub DRTM launch support for Secure Launch

On February 21, 2024 12:17:30 PM PST, ross.philipson@...cle.com wrote:
>On 2/15/24 1:01 AM, Ard Biesheuvel wrote:
>> On Wed, 14 Feb 2024 at 23:32, Ross Philipson <ross.philipson@...cle.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@...cle.com>
>>> ---
>>>   drivers/firmware/efi/libstub/x86-stub.c | 55 +++++++++++++++++++++++++
>>>   1 file changed, 55 insertions(+)
>>> 
>>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>>> index 0d510c9a06a4..4df2cf539194 100644
>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>> @@ -9,6 +9,7 @@
>>>   #include <linux/efi.h>
>>>   #include <linux/pci.h>
>>>   #include <linux/stddef.h>
>>> +#include <linux/slr_table.h>
>>> 
>>>   #include <asm/efi.h>
>>>   #include <asm/e820/types.h>
>>> @@ -810,6 +811,57 @@ static efi_status_t efi_decompress_kernel(unsigned long *kernel_entry)
>>>          return EFI_SUCCESS;
>>>   }
>>> 
>>> +static void efi_secure_launch(struct boot_params *boot_params)
>>> +{
>>> +       struct slr_entry_uefi_config *uefi_config;
>>> +       struct slr_uefi_cfg_entry *uefi_entry;
>>> +       struct slr_entry_dl_info *dlinfo;
>>> +       efi_guid_t guid = SLR_TABLE_GUID;
>>> +       struct slr_table *slrt;
>>> +       u64 memmap_hi;
>>> +       void *table;
>>> +       u8 buf[64] = {0};
>>> +
>> 
>> If you add a flex array to slr_entry_uefi_config as I suggested in
>> response to the other patch, we could simplify this substantially
>
>I feel like there is some reason why we did not use flex arrays. We were talking and we seem to remember we used to use them and someone asked us to remove them. We are still looking into it. But if we can go back to them, I will take all the changes you recommended here.
>
>Thanks
>Ross
>
>> 
>> static struct slr_entry_uefi_config cfg = {
>>          .hdr.tag        = SLR_ENTRY_UEFI_CONFIG,
>>          .hdr.size       = sizeof(cfg),
>>          .revision       = SLR_UEFI_CONFIG_REVISION,
>>          .nr_entries     = 1,
>>          .entries[0]     = {
>>                  .pcr    = 18,
>>                  .evt_info = "Measured UEFI memory map",
>>          },
>> };
>> 
>> cfg.entries[0].cfg  = boot_params->efi_info.efi_memmap |
>>                        (u64)boot_params->efi_info.efi_memmap_hi << 32;
>> cfg.entries[0].size = boot_params->efi_info.efi_memmap_size;
>> 
>> 
>> 
>>> +       table = get_efi_config_table(guid);
>>> +
>>> +       /*
>>> +        * The presence of this table indicated a Secure Launch
>>> +        * is being requested.
>>> +        */
>>> +       if (!table)
>>> +               return;
>>> +
>>> +       slrt = (struct slr_table *)table;
>>> +
>>> +       if (slrt->magic != SLR_TABLE_MAGIC)
>>> +               return;
>>> +
>> 
>> slrt = (struct slr_table *)get_efi_config_table(guid);
>> if (!slrt || slrt->magic != SLR_TABLE_MAGIC)
>>          return;
>> 
>>> +       /* Add config information to measure the UEFI memory map */
>>> +       uefi_config = (struct slr_entry_uefi_config *)buf;
>>> +       uefi_config->hdr.tag = SLR_ENTRY_UEFI_CONFIG;
>>> +       uefi_config->hdr.size = sizeof(*uefi_config) + sizeof(*uefi_entry);
>>> +       uefi_config->revision = SLR_UEFI_CONFIG_REVISION;
>>> +       uefi_config->nr_entries = 1;
>>> +       uefi_entry = (struct slr_uefi_cfg_entry *)(buf + sizeof(*uefi_config));
>>> +       uefi_entry->pcr = 18;
>>> +       uefi_entry->cfg = boot_params->efi_info.efi_memmap;
>>> +       memmap_hi = boot_params->efi_info.efi_memmap_hi;
>>> +       uefi_entry->cfg |= memmap_hi << 32;
>>> +       uefi_entry->size = boot_params->efi_info.efi_memmap_size;
>>> +       memcpy(&uefi_entry->evt_info[0], "Measured UEFI memory map",
>>> +               strlen("Measured UEFI memory map"));
>>> +
>> 
>> Drop all of this
>> 
>>> +       if (slr_add_entry(slrt, (struct slr_entry_hdr *)uefi_config))
>> 
>> if (slr_add_entry(slrt, &uefi_config.hdr))
>> 
>> 
>>> +               return;
>>> +
>>> +       /* Jump through DL stub to initiate Secure Launch */
>>> +       dlinfo = (struct slr_entry_dl_info *)
>>> +               slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_DL_INFO);
>>> +
>>> +       asm volatile ("jmp *%%rax"
>>> +                     : : "a" (dlinfo->dl_handler), "D" (&dlinfo->bl_context));
>> 
>> Fix the prototype and just do
>> 
>> dlinfo->dl_handler(&dlinfo->bl_context);
>> unreachable();
>> 
>> 
>> So in summary, this becomes
>> 
>> static void efi_secure_launch(struct boot_params *boot_params)
>> {
>>          static struct slr_entry_uefi_config cfg = {
>>                  .hdr.tag        = SLR_ENTRY_UEFI_CONFIG,
>>                  .hdr.size       = sizeof(cfg),
>>                  .revision       = SLR_UEFI_CONFIG_REVISION,
>>                  .nr_entries     = 1,
>>                  .entries[0]     = {
>>                          .pcr    = 18,
>>                          .evt_info = "Measured UEFI memory map",
>>                  },
>>          };
>>          struct slr_entry_dl_info *dlinfo;
>>          efi_guid_t guid = SLR_TABLE_GUID;
>>          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;
>> 
>>          cfg.entries[0].cfg  = boot_params->efi_info.efi_memmap |
>>                                (u64)boot_params->efi_info.efi_memmap_hi << 32;
>>          cfg.entries[0].size = boot_params->efi_info.efi_memmap_size;
>> 
>>          if (slr_add_entry(slrt, &cfg.hdr))
>>                  return;
>> 
>>          /* Jump through DL stub to initiate Secure Launch */
>>          dlinfo = (struct slr_entry_dl_info *)
>>                   slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_DL_INFO);
>> 
>>          dlinfo->dl_handler(&dlinfo->bl_context);
>> 
>>          unreachable();
>> }
>> 
>> 
>>> +}
>>> +
>>>   static void __noreturn enter_kernel(unsigned long kernel_addr,
>>>                                      struct boot_params *boot_params)
>>>   {
>>> @@ -934,6 +986,9 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>>>                  goto fail;
>>>          }
>>> 
>>> +       /* If a Secure Launch is in progress, this never returns */
>> 
>> if (IS_ENABLED(CONFIG_SECURE_LAUNCH))
>> 
>>> +       efi_secure_launch(boot_params);
>>> +
>>>          /*
>>>           * 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
>>> 
>> 
>

Linux kernel code doesn't use VLAs because of the limited stack size, and VLAs or alloca() makes stack size tracking impossible. Although this technically speaking runs in a different environment, it is easier to enforce the constraint globally.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ