| Message ID | 20210707181506.30489-23-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
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 <michael.roth@amd.com> > > 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 <michael.roth@amd.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > 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...
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 <michael.roth@amd.com> > > > > 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 <michael.roth@amd.com> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > > --- > > 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&data=04%7C01%7Cmichael.roth%40amd.com%7C567fab11117b4072171508d962f6043a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649631103094962%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fg87GYa5RX5ea54IwYLzwXupt6VVyLM%2BkyMnGB3S0wQ%3D&reserved=0
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? > "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. > 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.
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
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.
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