Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <YNwu7LmZaImyoOer@zn.tnic>
In-Reply-To: <YNwu7LmZaImyoOer@zn.tnic>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Fri, 16 Jul 2021 19:10:17 +0200
Message-ID: <CAMj1kXE--H53wu_X=GpgeJmWs7vjpnkUnG_fc+59GaNDF+sYEw@mail.gmail.com>
Subject: Re: [RFC PATCH] efi/mokvar: Reserve the table only if it is in boot
 services data
To:     Borislav Petkov <bp@alien8.de>
Cc:     Lenny Szubowicz <lszubowi@redhat.com>, Gary Lin <glin@suse.com>,
        =?UTF-8?B?SsO2cmcgUsO2ZGVs?= <jroedel@suse.de>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        linux-efi <linux-efi@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 30 Jun 2021 at 10:44, Borislav Petkov <bp@alien8.de> wrote:
>
> Hi guys,
>
> so below is what we've been staring at recently, please doublecheck me
> whether I'm even making sense here.
>
> Thx!
>
> ---
> From: Borislav Petkov <bp@suse.de>
>
> One of the SUSE QA tests triggered:
>
>   localhost kernel: efi: Failed to lookup EFI memory descriptor for 0x000000003dcf8000
>
> which comes from x86's version of efi_arch_mem_reserve() trying to
> reserve a memory region. Usually, that function expects
> EFI_BOOT_SERVICES_DATA memory descriptors but the above case is for the
> MOKvar table which is allocated in the EFI shim as runtime services.
>
> That lead to a fix changing the allocation of that table to boot services.
>
> However, that fix broke booting SEV guests with that shim leading to
> this kernel fix
>
>   8d651ee9c71b ("x86/ioremap: Map EFI-reserved memory as encrypted for SEV")
>
> which extended the ioremap hint to map reserved EFI boot services as
> decrypted too.
>
> However, all that wasn't needed, IMO, because that error message in
> efi_arch_mem_reserve() was innocuous in this case - if the MOKvar table
> is not in boot services, then it doesn't need to be reserved in the
> first place because it is, well, in runtime services which *should* be
> reserved anyway.
>
> So do that reservation for the MOKvar table only if it is allocated
> in boot services data. I couldn't find any requirement about where
> that table should be allocated in, unlike the ESRT which allocation is
> mandated to be done in boot services data by the UEFI spec.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

Would you like me to queue this as a fix?

> ---
>  drivers/firmware/efi/mokvar-table.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c
> index d8bc01340686..38722d2009e2 100644
> --- a/drivers/firmware/efi/mokvar-table.c
> +++ b/drivers/firmware/efi/mokvar-table.c
> @@ -180,7 +180,10 @@ void __init efi_mokvar_table_init(void)
>                 pr_err("EFI MOKvar config table is not valid\n");
>                 return;
>         }
> -       efi_mem_reserve(efi.mokvar_table, map_size_needed);
> +
> +       if (md.type == EFI_BOOT_SERVICES_DATA)
> +               efi_mem_reserve(efi.mokvar_table, map_size_needed);
> +
>         efi_mokvar_table_size = map_size_needed;
>  }
>
> --
> 2.29.2
>
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette