Received-SPF: pass (google.com: domain of linux-crypto+bounces-2080-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240214221847.2066632-1-ross.philipson@oracle.com> <20240214221847.2066632-16-ross.philipson@oracle.com>
In-Reply-To: <20240214221847.2066632-16-ross.philipson@oracle.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Thu, 15 Feb 2024 10:01:03 +0100
Message-ID: <CAMj1kXF3k_c4Wn9GU+NC_+_aYfDpAzAUnfR=A4L_T+re1H3G=w@mail.gmail.com>
Subject: Re: [PATCH v8 15/15] x86: EFI stub DRTM launch support for Secure Launch
To: Ross Philipson <ross.philipson@oracle.com>
Cc: 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, hpa@zytor.com, 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
Content-Type: text/plain; charset="UTF-8"

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
>