linux-kernel - Re: [PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI stub

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: <89b28d47-bc33-456d-9b8a-a8d4ec27da84@amd.com>
Date: Fri, 12 Sep 2025 08:32:30 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Ard Biesheuvel <ardb+git@...gle.com>, linux-efi@...r.kernel.org,
 linux-kernel@...r.kernel.org, x86@...nel.org, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI
 stub

On 9/12/25 03:26, Ard Biesheuvel wrote:
> On Fri, 12 Sept 2025 at 09:29, Ard Biesheuvel <ardb@...nel.org> wrote:
>>
>> On Thu, 11 Sept 2025 at 23:53, Tom Lendacky <thomas.lendacky@....com> wrote:
>>>
>>> On 9/9/25 03:06, Ard Biesheuvel wrote:
>>>> From: Ard Biesheuvel <ardb@...nel.org>
>>>>
>>>> One of the last things the EFI stub does before handing over to the core
>>>> kernel when booting as a SEV guest is enabling SEV, even though this is
>>>> mostly redundant: one of the first things the core kernel does is
>>>> calling sme_enable(), after setting up the early GDT and IDT but before
>>>> even setting up the kernel page tables. sme_enable() performs the same
>>>> SEV-SNP initialization that the decompressor performs in sev_enable().
>>>>
>>>> So let's just drop this call to sev_enable(), and rely on the core
>>>> kernel to initiaize SEV correctly.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ardb@...nel.org>
>>>> ---
>>>>  arch/x86/include/asm/sev.h              | 2 --
>>>>  drivers/firmware/efi/libstub/x86-stub.c | 6 ------
>>>>  2 files changed, 8 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>>>> index d7be1ff3f7e0..b017e1dab705 100644
>>>> --- a/arch/x86/include/asm/sev.h
>>>> +++ b/arch/x86/include/asm/sev.h
>>>> @@ -462,7 +462,6 @@ static __always_inline void sev_es_nmi_complete(void)
>>>>               __sev_es_nmi_complete();
>>>>  }
>>>>  extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
>>>> -extern void sev_enable(struct boot_params *bp);
>>>>
>>>>  /*
>>>>   * RMPADJUST modifies the RMP permissions of a page of a lesser-
>>>> @@ -588,7 +587,6 @@ static inline void sev_es_ist_exit(void) { }
>>>>  static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
>>>>  static inline void sev_es_nmi_complete(void) { }
>>>>  static inline int sev_es_efi_map_ghcbs_cas(pgd_t *pgd) { return 0; }
>>>> -static inline void sev_enable(struct boot_params *bp) { }
>>>>  static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
>>>>  static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
>>>>  static inline void setup_ghcb(void) { }
>>>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>>>> index c4ef645762ec..354bc3901193 100644
>>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>>> @@ -938,12 +938,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>>>>               goto fail;
>>>>       }
>>>>
>>>> -     /*
>>>> -      * Call the SEV init code while still running with the firmware's
>>>> -      * GDT/IDT, so #VC exceptions will be handled by EFI.
>>>> -      */
>>>> -     sev_enable(boot_params);
>>>
>>> I think we lose the check for GHCB_HV_FT_SNP_MULTI_VMPL by doing this. It
>>> might need move into svsm_setup_ca() now.
>>>
>>
>> Currently, this check only occurs inside sev_enable(), and so it
>> happens too late to have an impact, given that the core kernel will
>> set up all of this state from scratch right away.
>>
> 
> Hmm, I only just spotted that this check only happens in the legacy
> decompressor.
> 
> I think it makes sense for this check to live in svsm_setup_ca(), but
> what is your take on the need to perform this check when accepting
> memory from the stub using the CA address obtained from the firmware?
> (i.e., way before sev_enable() is called)

Yes, it seems like it should be checked before memory acceptance if we're
using an SVSM, so early_is_sevsnp_guest() looks appropriate. But since
this may not be called, the check also has to be performed by the core
kernel in svsm_setup_ca(), too.

Just wondering if it is truly necessary to check in the stub just for this
case. Theoretically, the SVSM should have validated that the HV has the
necessary support before ever invoking the firmware. Just putting it in
svsm_setup_ca() seems enough to me.

@Boris, what do you think?

Thanks,
Tom