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

Commit Message

Brijesh Singh July 7, 2021, 6:14 p.m. UTC

From: Michael Roth <[e-mail]>

This code will also be used later for SEV-SNP-validated CPUID code in
some cases, so move it to a common helper.

Signed-off-by: Michael Roth <[e-mail]>
Signed-off-by: Brijesh Singh <[e-mail]>
---
 arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 26 deletions(-)

Comments

Borislav Petkov Aug. 19, 2021, 9:45 a.m. UTC | #1

Finally drop this bouncing npmccallum at RH email address from the Cc
list.

On Wed, Jul 07, 2021 at 01:14:52PM -0500, Brijesh Singh wrote:
> From: Michael Roth <[e-mail]>
> 
> This code will also be used later for SEV-SNP-validated CPUID code in
> some cases, so move it to a common helper.
> 
> Signed-off-by: Michael Roth <[e-mail]>
> Signed-off-by: Brijesh Singh <[e-mail]>
> ---
>  arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index be4025f14b4f..4884de256a49 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -184,6 +184,58 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
>  	return ret;
>  }
>  
> +static int sev_es_cpuid_msr_proto(u32 func, u32 subfunc, u32 *eax, u32 *ebx,

Since it is not only SEV-ES, then it should be prefixed with "sev_" like
we do for the other such functions. I guess simply

	sev_cpuid()

?

> +				  u32 *ecx, u32 *edx)
> +{
> +	u64 val;
> +
> +	if (eax) {

What's the protection for? Is it ever going to be called with NULL ptrs
for the regs? That's not the case in this patchset at least...

Michael Roth Aug. 19, 2021, 3:37 p.m. UTC | #2

On Thu, Aug 19, 2021 at 11:45:35AM +0200, Borislav Petkov wrote:
> Finally drop this bouncing npmccallum at RH email address from the Cc
> list.
> 
> On Wed, Jul 07, 2021 at 01:14:52PM -0500, Brijesh Singh wrote:
> > From: Michael Roth <[e-mail]>
> > 
> > This code will also be used later for SEV-SNP-validated CPUID code in
> > some cases, so move it to a common helper.
> > 
> > Signed-off-by: Michael Roth <[e-mail]>
> > Signed-off-by: Brijesh Singh <[e-mail]>
> > ---
> >  arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
> >  1 file changed, 58 insertions(+), 26 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> > index be4025f14b4f..4884de256a49 100644
> > --- a/arch/x86/kernel/sev-shared.c
> > +++ b/arch/x86/kernel/sev-shared.c
> > @@ -184,6 +184,58 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
> >  	return ret;
> >  }
> >  
> > +static int sev_es_cpuid_msr_proto(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> 
> Since it is not only SEV-ES, then it should be prefixed with "sev_" like
> we do for the other such functions. I guess simply
> 
> 	sev_cpuid()
> 
> ?

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. "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?

> 
> > +				  u32 *ecx, u32 *edx)
> > +{
> > +	u64 val;
> > +
> > +	if (eax) {
> 
> What's the protection for? Is it ever going to be called with NULL ptrs
> for the regs? That's not the case in this patchset at least...

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.

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C567fab11117b4072171508d962f6043a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649631103094962%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=fg87GYa5RX5ea54IwYLzwXupt6VVyLM%2BkyMnGB3S0wQ%3D&amp;reserved=0

Michael Roth Aug. 20, 2021, 3:29 a.m. UTC | #3

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

Borislav Petkov Aug. 23, 2021, 4:50 a.m. UTC | #4

On Thu, Aug 19, 2021 at 10:29:08PM -0500, Michael Roth wrote:
> 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.

Ok, makes sense.

Thx.

Patch

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index be4025f14b4f..4884de256a49 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -184,6 +184,58 @@  static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
 	return ret;
 }
 
+static int sev_es_cpuid_msr_proto(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
+				  u32 *ecx, u32 *edx)
+{
+	u64 val;
+
+	if (eax) {
+		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EAX));
+		VMGEXIT();
+		val = sev_es_rd_ghcb_msr();
+
+		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+			return -EIO;
+
+		*eax = (val >> 32);
+	}
+
+	if (ebx) {
+		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EBX));
+		VMGEXIT();
+		val = sev_es_rd_ghcb_msr();
+
+		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+			return -EIO;
+
+		*ebx = (val >> 32);
+	}
+
+	if (ecx) {
+		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_ECX));
+		VMGEXIT();
+		val = sev_es_rd_ghcb_msr();
+
+		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+			return -EIO;
+
+		*ecx = (val >> 32);
+	}
+
+	if (edx) {
+		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EDX));
+		VMGEXIT();
+		val = sev_es_rd_ghcb_msr();
+
+		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+			return -EIO;
+
+		*edx = (val >> 32);
+	}
+
+	return 0;
+}
+
 /*
  * Boot VC Handler - This is the first VC handler during boot, there is no GHCB
  * page yet, so it only supports the MSR based communication with the
@@ -192,39 +244,19 @@  static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
 void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
 {
 	unsigned int fn = lower_bits(regs->ax, 32);
-	unsigned long val;
+	u32 eax, ebx, ecx, edx;
 
 	/* Only CPUID is supported via MSR protocol */
 	if (exit_code != SVM_EXIT_CPUID)
 		goto fail;
 
-	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EAX));
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+	if (sev_es_cpuid_msr_proto(fn, 0, &eax, &ebx, &ecx, &edx))
 		goto fail;
-	regs->ax = val >> 32;
 
-	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EBX));
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
-		goto fail;
-	regs->bx = val >> 32;
-
-	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_ECX));
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
-		goto fail;
-	regs->cx = val >> 32;
-
-	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EDX));
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
-		goto fail;
-	regs->dx = val >> 32;
+	regs->ax = eax;
+	regs->bx = ebx;
+	regs->cx = ecx;
+	regs->dx = edx;
 
 	/*
 	 * This is a VC handler and the #VC is only raised when SEV-ES is