[v8,15/15] x86: EFI stub DRTM launch support for Secure Launch

  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 | 55 +++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
  

On Wed, 14 Feb 2024 at 23: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>
> ---
>  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

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
>
  

Hi Ross,

kernel test robot noticed the following build errors:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master v6.8-rc4 next-20240216]
[cannot apply to tip/x86/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ross-Philipson/x86-boot-Place-kernel_info-at-a-fixed-offset/20240215-064712
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20240214221847.2066632-16-ross.philipson%40oracle.com
patch subject: [PATCH v8 15/15] x86: EFI stub DRTM launch support for Secure Launch
config: i386-randconfig-052-20240215 (https://download.01.org/0day-ci/archive/20240217/202402171503.kLhNHtkM-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240217/202402171503.kLhNHtkM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402171503.kLhNHtkM-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/firmware/efi/libstub/x86-stub.c:862:18: error: invalid input size for constraint 'a'
     862 |                       : : "a" (dlinfo->dl_handler), "D" (&dlinfo->bl_context));
         |                                ^
   1 error generated.


vim +/a +862 drivers/firmware/efi/libstub/x86-stub.c

   813	
   814	static void efi_secure_launch(struct boot_params *boot_params)
   815	{
   816		struct slr_entry_uefi_config *uefi_config;
   817		struct slr_uefi_cfg_entry *uefi_entry;
   818		struct slr_entry_dl_info *dlinfo;
   819		efi_guid_t guid = SLR_TABLE_GUID;
   820		struct slr_table *slrt;
   821		u64 memmap_hi;
   822		void *table;
   823		u8 buf[64] = {0};
   824	
   825		table = get_efi_config_table(guid);
   826	
   827		/*
   828		 * The presence of this table indicated a Secure Launch
   829		 * is being requested.
   830		 */
   831		if (!table)
   832			return;
   833	
   834		slrt = (struct slr_table *)table;
   835	
   836		if (slrt->magic != SLR_TABLE_MAGIC)
   837			return;
   838	
   839		/* Add config information to measure the UEFI memory map */
   840		uefi_config = (struct slr_entry_uefi_config *)buf;
   841		uefi_config->hdr.tag = SLR_ENTRY_UEFI_CONFIG;
   842		uefi_config->hdr.size = sizeof(*uefi_config) + sizeof(*uefi_entry);
   843		uefi_config->revision = SLR_UEFI_CONFIG_REVISION;
   844		uefi_config->nr_entries = 1;
   845		uefi_entry = (struct slr_uefi_cfg_entry *)(buf + sizeof(*uefi_config));
   846		uefi_entry->pcr = 18;
   847		uefi_entry->cfg = boot_params->efi_info.efi_memmap;
   848		memmap_hi = boot_params->efi_info.efi_memmap_hi;
   849		uefi_entry->cfg |= memmap_hi << 32;
   850		uefi_entry->size = boot_params->efi_info.efi_memmap_size;
   851		memcpy(&uefi_entry->evt_info[0], "Measured UEFI memory map",
   852			strlen("Measured UEFI memory map"));
   853	
   854		if (slr_add_entry(slrt, (struct slr_entry_hdr *)uefi_config))
   855			return;
   856	
   857		/* Jump through DL stub to initiate Secure Launch */
   858		dlinfo = (struct slr_entry_dl_info *)
   859			slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_DL_INFO);
   860	
   861		asm volatile ("jmp *%%rax"
 > 862			      : : "a" (dlinfo->dl_handler), "D" (&dlinfo->bl_context));
   863	}
   864
  

Hi Ross,

kernel test robot noticed the following build errors:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master v6.8-rc4 next-20240216]
[cannot apply to tip/x86/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ross-Philipson/x86-boot-Place-kernel_info-at-a-fixed-offset/20240215-064712
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20240214221847.2066632-16-ross.philipson%40oracle.com
patch subject: [PATCH v8 15/15] x86: EFI stub DRTM launch support for Secure Launch
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240218/202402180324.a3PqEegg-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240218/202402180324.a3PqEegg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402180324.a3PqEegg-lkp@intel.com/

All errors (new ones prefixed by >>):

   In function 'efi_secure_launch',
       inlined from 'efi_stub_entry' at drivers/firmware/efi/libstub/x86-stub.c:990:2:
>> drivers/firmware/efi/libstub/x86-stub.c:861:9: error: inconsistent operand constraints in an 'asm'
     861 |         asm volatile ("jmp *%%rax"
         |         ^~~


vim +/asm +861 drivers/firmware/efi/libstub/x86-stub.c

   813	
   814	static void efi_secure_launch(struct boot_params *boot_params)
   815	{
   816		struct slr_entry_uefi_config *uefi_config;
   817		struct slr_uefi_cfg_entry *uefi_entry;
   818		struct slr_entry_dl_info *dlinfo;
   819		efi_guid_t guid = SLR_TABLE_GUID;
   820		struct slr_table *slrt;
   821		u64 memmap_hi;
   822		void *table;
   823		u8 buf[64] = {0};
   824	
   825		table = get_efi_config_table(guid);
   826	
   827		/*
   828		 * The presence of this table indicated a Secure Launch
   829		 * is being requested.
   830		 */
   831		if (!table)
   832			return;
   833	
   834		slrt = (struct slr_table *)table;
   835	
   836		if (slrt->magic != SLR_TABLE_MAGIC)
   837			return;
   838	
   839		/* Add config information to measure the UEFI memory map */
   840		uefi_config = (struct slr_entry_uefi_config *)buf;
   841		uefi_config->hdr.tag = SLR_ENTRY_UEFI_CONFIG;
   842		uefi_config->hdr.size = sizeof(*uefi_config) + sizeof(*uefi_entry);
   843		uefi_config->revision = SLR_UEFI_CONFIG_REVISION;
   844		uefi_config->nr_entries = 1;
   845		uefi_entry = (struct slr_uefi_cfg_entry *)(buf + sizeof(*uefi_config));
   846		uefi_entry->pcr = 18;
   847		uefi_entry->cfg = boot_params->efi_info.efi_memmap;
   848		memmap_hi = boot_params->efi_info.efi_memmap_hi;
   849		uefi_entry->cfg |= memmap_hi << 32;
   850		uefi_entry->size = boot_params->efi_info.efi_memmap_size;
   851		memcpy(&uefi_entry->evt_info[0], "Measured UEFI memory map",
   852			strlen("Measured UEFI memory map"));
   853	
   854		if (slr_add_entry(slrt, (struct slr_entry_hdr *)uefi_config))
   855			return;
   856	
   857		/* Jump through DL stub to initiate Secure Launch */
   858		dlinfo = (struct slr_entry_dl_info *)
   859			slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_DL_INFO);
   860	
 > 861		asm volatile ("jmp *%%rax"
   862			      : : "a" (dlinfo->dl_handler), "D" (&dlinfo->bl_context));
   863	}
   864
  

On 2/15/24 1:01 AM, Ard Biesheuvel wrote:
> On Wed, 14 Feb 2024 at 23: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>
>> ---
>>   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
>>
>
  

On February 21, 2024 12:17:30 PM PST, ross.philipson@oracle.com wrote:
>On 2/15/24 1:01 AM, Ard Biesheuvel wrote:
>> On Wed, 14 Feb 2024 at 23: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>
>>> ---
>>>   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.
  

On Wed, 21 Feb 2024 at 21:37, H. Peter Anvin <hpa@zytor.com> wrote:
>
> On February 21, 2024 12:17:30 PM PST, ross.philipson@oracle.com wrote:
> >On 2/15/24 1:01 AM, Ard Biesheuvel wrote:
> >> On Wed, 14 Feb 2024 at 23: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>
> >>> ---
> >>>   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.
> >
>
> 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.

Flex array != VLA

VLAs were phased out because of this reason (and VLAISs [VLAs in
structs] were phased out before that because they are a GNU extension
and not supported by Clang)

Today, VLAs are not supported anywhere in the kernel.

Flex arrays are widely used in the kernel. A flex array is a trailing
array of unspecified size in a struct that makes the entire *type*
have a variable size. But that does not make them VLAs (or VLAISs) - a
VLA is a stack allocated *variable* whose size is based on a function
parameter.

Instances of types containing flex arrays can be allocated statically,
or dynamically on the heap. This is common practice in the kernel, and
even supported by instrumentation to help the compiler track the
runtime size and flag overruns. We are even in the process of adding
compiler support to annotate struct members as carrying the number of
elements in an associated flex arrays, to improve the coverage of the
instrumentation.

I am not asking for a VLA here, only a flex array.