linux-kernel - Re: [RFT PATCH v3 12/21] x86/sev: Unify SEV-SNP hypervisor feature check

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: <CAMj1kXEzbBTYr2vF3g_z49muSTHwRY1QUm2iOFsaLNYuijqHDg@mail.gmail.com>
Date: Fri, 30 May 2025 16:28:52 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Borislav Petkov <bp@...en8.de>
Cc: Ard Biesheuvel <ardb+git@...gle.com>, linux-kernel@...r.kernel.org, 
	linux-efi@...r.kernel.org, x86@...nel.org, Ingo Molnar <mingo@...nel.org>, 
	Dionna Amalie Glaze <dionnaglaze@...gle.com>, Kevin Loughlin <kevinloughlin@...gle.com>, 
	Tom Lendacky <thomas.lendacky@....com>
Subject: Re: [RFT PATCH v3 12/21] x86/sev: Unify SEV-SNP hypervisor feature check

On Fri, 30 May 2025 at 13:17, Borislav Petkov <bp@...en8.de> wrote:
>
> On Mon, May 12, 2025 at 09:08:47PM +0200, Ard Biesheuvel wrote:
> > diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
> > index 70ad9a0aa023..560985ef8df6 100644
> > --- a/arch/x86/boot/startup/sev-shared.c
> > +++ b/arch/x86/boot/startup/sev-shared.c
> > @@ -66,16 +66,10 @@ sev_es_terminate(unsigned int set, unsigned int reason)
> >               asm volatile("hlt\n" : : : "memory");
> >  }
> >
> > -/*
> > - * The hypervisor features are available from GHCB version 2 onward.
> > - */
> > -u64 get_hv_features(void)
> > +static u64 __head get_hv_features(void)
> >  {
> >       u64 val;
> >
> > -     if (ghcb_version < 2)
> > -             return 0;
>
> Why?
>

It is explained below ...
> > -
> >       sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
> >       VMGEXIT();
> >
> > @@ -86,6 +80,31 @@ u64 get_hv_features(void)
> >       return GHCB_MSR_HV_FT_RESP_VAL(val);
> >  }
> >
> > +u64 __head snp_check_hv_features(void)
> > +{
> > +     /*
> > +      * SNP is supported in v2 of the GHCB spec which mandates support for HV
> > +      * features.
> > +      */

... get_hv_features() is only when SEV-SNP has already been detected.

> > +     if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
> > +             u64 hv_features;
> > +
> > +             hv_features = get_hv_features();
> > +             if (!(hv_features & GHCB_HV_FT_SNP))
> > +                     sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> > +
> > +             /*
> > +              * Running at VMPL0 is required unless an SVSM is present and
> > +              * the hypervisor supports the required SVSM GHCB events.
> > +              */
> > +             if (snp_vmpl > 0 && !(hv_features & GHCB_HV_FT_SNP_MULTI_VMPL))
> > +                     sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
> > +
> > +             return hv_features;
> > +     }
> > +     return 0;
> > +}
> > +
> >  int svsm_perform_msr_protocol(struct svsm_call *call)
> >  {
> >       u8 pending = 0;
> > diff --git a/arch/x86/boot/startup/sme.c b/arch/x86/boot/startup/sme.c
> > index 753cd2094080..0ae04e333f51 100644
> > --- a/arch/x86/boot/startup/sme.c
> > +++ b/arch/x86/boot/startup/sme.c
> > @@ -533,6 +533,8 @@ void __head sme_enable(struct boot_params *bp)
> >       if (snp_en ^ !!(msr & MSR_AMD64_SEV_SNP_ENABLED))
> >               snp_abort();
> >
> > +     sev_hv_features = snp_check_hv_features();
>
> Is this writing the sev_hv_features declared in
>
> arch/x86/boot/startup/sev-startup.c
>
> ?
>
> arch/x86/boot/startup/sev-startup.c:45:u64 sev_hv_features __ro_after_init;
> arch/x86/boot/startup/sme.c:536:        sev_hv_features = snp_check_hv_features();
> arch/x86/coco/sev/core.c:980:   if (!(sev_hv_features & GHCB_HV_FT_SNP_AP_CREATION))
> arch/x86/include/asm/sev-internal.h:5:extern u64 sev_hv_features;
> arch/x86/include/asm/sev.h:428:extern u64 sev_hv_features;
>
> I'm confused.
>
> If sev_hv_features is startup code, why isn't it accessed this way...?
>
> /me goes and looks forward in the set...
>
> oh my, that is coming.
>
> > +
> >       /* Check if memory encryption is enabled */
> >       if (feature_mask == AMD_SME_BIT) {
> >               if (!(bp->hdr.xloadflags & XLF_MEM_ENCRYPTION))
> > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> > index fa7fdd11a45b..fc4f6f188d42 100644
> > --- a/arch/x86/coco/sev/core.c
> > +++ b/arch/x86/coco/sev/core.c
> > @@ -1264,17 +1264,6 @@ void __init sev_es_init_vc_handling(void)
> >       if (!sev_es_check_cpu_features())
> >               panic("SEV-ES CPU Features missing");
> >
> > -     /*
> > -      * SNP is supported in v2 of the GHCB spec which mandates support for HV
> > -      * features.
> > -      */
> > -     if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
> > -             sev_hv_features = get_hv_features();
> > -
> > -             if (!(sev_hv_features & GHCB_HV_FT_SNP))
> > -                     sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> > -     }
>
> I guess we would've terminated earlier anyway so that's probably ok...
>
> > -
> >       /* Initialize per-cpu GHCB pages */
> >       for_each_possible_cpu(cpu) {
> >               alloc_runtime_data(cpu);
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette