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

From: Ard Biesheuvel <ardb@kernel.org>
To: "H. Peter Anvin" <hpa@zytor.com>, Kees Cook <keescook@chromium.org>
Cc: ross.philipson@oracle.com, linux-kernel@vger.kernel.org,
	x86@kernel.org,  linux-integrity@vger.kernel.org,
	linux-doc@vger.kernel.org,  linux-crypto@vger.kernel.org,
	kexec@lists.infradead.org,  linux-efi@vger.kernel.org,
	dpsmith@apertussolutions.com, tglx@linutronix.de,
	 mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
	 mjg59@srcf.ucam.org, James.Bottomley@hansenpartnership.com,
	peterhuewe@gmx.de,  jarkko@kernel.org, jgg@ziepe.ca,
	luto@amacapital.net, nivedita@alum.mit.edu,
	 herbert@gondor.apana.org.au, davem@davemloft.net,
	kanth.ghatraju@oracle.com,  trenchboot-devel@googlegroups.com
Subject: Re: [PATCH v8 15/15] x86: EFI stub DRTM launch support for Secure Launch
Date: Thu, 22 Feb 2024 00:24:06 +0100	[thread overview]
Message-ID: <CAMj1kXFYVbCd3EaweseNndhmOwdbzEmvB1vjWk2rmTBxAoPCxg@mail.gmail.com> (raw)
In-Reply-To: <C98F883A-31D5-4F67-97FF-4AEFAADDDC74@zytor.com>

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.