Re: [PATCH Part1 RFC v4 22/36] x86/sev: move MSR-based VMGEXITs for CPUID to helper

[Michael Roth]

On Thu, Aug 19, 2021 at 06:46:48PM +0200, Borislav Petkov wrote:
> On Thu, Aug 19, 2021 at 10:37:41AM -0500, Michael Roth wrote:
> > That makes sense, but I think it helps in making sense of the security
> > aspects of the code to know that sev_cpuid() would be fetching cpuid
> > information from the hypervisor.
> 
> Why is it important for the callers to know where do we fetch the CPUID
> info from?

The select cases where we still fetch CPUID values from hypervisor in
SNP need careful consideration, so for the purposes of auditing the code
for security, or just noticing things in patches, I think it's important
to make it clear what is the "normal" SNP case (not trusting hypervisor
CPUID values) and what are exceptional cases (getting select values from
hypervisor). If something got added in the future, I think something
like:

  +sev_cpuid_hv(0x8000001f, ...)

would be more likely to raise eyebrows and get more scrutiny than:

  +sev_cpuid(0x8000001f, ...)

where it might get lost in the noise or mistaken as similar to
sev_snp_cpuid().

Maybe a bit contrived, and probably not a big deal in practice, but
conveying the source it in the naming does seem at least seem slightly
better than not doing so.

> 
> > "msr_proto" is meant to be an indicator that it will be using the GHCB
> > MSR protocol to do it, but maybe just "_hyp" is enough to get the idea
> > across? I use the convention elsewhere in the series as well.
> >
> > So sev_cpuid_hyp() maybe?
> 
> sev_cpuid_hv() pls. We abbreviate the hypervisor as HV usually.

Ah yes, much nicer. I've gone with this for v5 and adopted the
convention in the rest of the code.

> 
> > In "enable SEV-SNP-validated CPUID in #VC handler", it does:
> >
> >   sev_snp_cpuid() -> sev_snp_cpuid_hyp(),
> >
> > which will call this with NULL e{a,b,c,d}x arguments in some cases. There
> > are enough call-sites in sev_snp_cpuid() that it seemed worthwhile to
> > add the guards so we wouldn't need to declare dummy variables for arguments.
> 
> Yah, saw that in the later patches.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7C6e23d0d9be7a4125d70008d96330de54%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649883863838712%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=HaRdEA0P4%2FGzmTXYyVYhGCnDaQHR8rbJqf%2B0xTBPSt0%3D&reserved=0