| 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
| ||
|
Message-ID: <20210827164601.fzr45veg7a6r4lbp@amd.com>
Date: Fri, 27 Aug 2021 11:46:01 -0500
From: Michael Roth <michael.roth@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: Brijesh Singh <brijesh.singh@....com>, x86@...nel.org,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
linux-efi@...r.kernel.org, platform-driver-x86@...r.kernel.org,
linux-coco@...ts.linux.dev, linux-mm@...ck.org,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
Tom Lendacky <thomas.lendacky@....com>,
"H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
Paolo Bonzini <pbonzini@...hat.com>,
Sean Christopherson <seanjc@...gle.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Andy Lutomirski <luto@...nel.org>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Sergio Lopez <slp@...hat.com>, Peter Gonda <pgonda@...gle.com>,
Peter Zijlstra <peterz@...radead.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
David Rientjes <rientjes@...gle.com>,
Dov Murik <dovmurik@...ux.ibm.com>,
Tobin Feldman-Fitzthum <tobin@....com>,
Vlastimil Babka <vbabka@...e.cz>,
"Kirill A . Shutemov" <kirill@...temov.name>,
Andi Kleen <ak@...ux.intel.com>, tony.luck@...el.com,
marcorr@...gle.com, sathyanarayanan.kuppuswamy@...ux.intel.com
Subject: Re: [PATCH Part1 v5 28/38] x86/compressed/64: enable
SEV-SNP-validated CPUID in #VC handler
On Wed, Aug 25, 2021 at 09:19:18PM +0200, Borislav Petkov wrote:
> On Fri, Aug 20, 2021 at 10:19:23AM -0500, Brijesh Singh wrote:
> > From: Michael Roth <michael.roth@....com>
> >
> > CPUID instructions generate a #VC exception for SEV-ES/SEV-SNP guests,
> > for which early handlers are currently set up to handle. In the case
> > of SEV-SNP, guests can use a special location in guest memory address
> > space that has been pre-populated with firmware-validated CPUID
> > information to look up the relevant CPUID values rather than
> > requesting them from hypervisor via a VMGEXIT.
> >
> > Determine the location of the CPUID memory address in advance of any
> > CPUID instructions/exceptions and, when available, use it to handle
> > the CPUID lookup.
> >
> > Signed-off-by: Michael Roth <michael.roth@....com>
> > Signed-off-by: Brijesh Singh <brijesh.singh@....com>
> > ---
> > arch/x86/boot/compressed/efi.c | 1 +
> > arch/x86/boot/compressed/head_64.S | 1 +
> > arch/x86/boot/compressed/idt_64.c | 7 +-
> > arch/x86/boot/compressed/misc.h | 1 +
> > arch/x86/boot/compressed/sev.c | 3 +
> > arch/x86/include/asm/sev-common.h | 2 +
> > arch/x86/include/asm/sev.h | 3 +
> > arch/x86/kernel/sev-shared.c | 374 +++++++++++++++++++++++++++++
> > arch/x86/kernel/sev.c | 4 +
> > 9 files changed, 394 insertions(+), 2 deletions(-)
>
> Another huuge patch. I wonder if it can be split...
I think I can split out at least sev_snp_cpuid_init() and
sev_snp_probe_cc_blob(). Adding the actual cpuid lookup and related code to
#VC handler though I'm not sure there's much that can be done there.
>
> > diff --git a/arch/x86/boot/compressed/efi.c b/arch/x86/boot/compressed/efi.c
> > index 16ff5cb9a1fb..a1529a230ea7 100644
> > --- a/arch/x86/boot/compressed/efi.c
> > +++ b/arch/x86/boot/compressed/efi.c
> > @@ -176,3 +176,4 @@ efi_get_conf_table(struct boot_params *boot_params,
> >
> > return 0;
> > }
> > +
>
> Applying: x86/compressed/64: Enable SEV-SNP-validated CPUID in #VC handler
> .git/rebase-apply/patch:21: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.
>
> That looks like a stray hunk which doesn't belong.
Will get this fixed up. I should've noticed these checkpatch warnings so
I modified my git hook to flag these a bit more prevalently.
>
> > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> > index a2347ded77ea..1c1658693fc9 100644
> > --- a/arch/x86/boot/compressed/head_64.S
> > +++ b/arch/x86/boot/compressed/head_64.S
> > @@ -441,6 +441,7 @@ SYM_CODE_START(startup_64)
> > .Lon_kernel_cs:
> >
> > pushq %rsi
> > + movq %rsi, %rdi /* real mode address */
> > call load_stage1_idt
> > popq %rsi
> >
> > diff --git a/arch/x86/boot/compressed/idt_64.c b/arch/x86/boot/compressed/idt_64.c
> > index 9b93567d663a..1f6511a6625d 100644
> > --- a/arch/x86/boot/compressed/idt_64.c
> > +++ b/arch/x86/boot/compressed/idt_64.c
> > @@ -3,6 +3,7 @@
> > #include <asm/segment.h>
> > #include <asm/trapnr.h>
> > #include "misc.h"
> > +#include <asm/sev.h>
>
> asm/ namespaced headers should go together, before the private ones,
> i.e., above the misc.h line.
Will make sure to great these together, but there seems to be a convention
of including misc.h first, since it does some fixups for subsequent
includes. So maybe that should be moved to the top? There's a comment in
boot/compressed/sev.c:
/*
* misc.h needs to be first because it knows how to include the other kernel
* headers in the pre-decompression code in a way that does not break
* compilation.
*/
And while it's not an issue here, asm/sev.h now needs to have
__BOOT_COMPRESSED #define'd in advance. So maybe that #define should be
moved into misc.h so it doesn't have to happen before each include?
>
> > static void set_idt_entry(int vector, void (*handler)(void))
> > {
> > @@ -28,13 +29,15 @@ static void load_boot_idt(const struct desc_ptr *dtr)
> > }
> >
> > /* Setup IDT before kernel jumping to .Lrelocated */
> > -void load_stage1_idt(void)
> > +void load_stage1_idt(void *rmode)
> > {
> > boot_idt_desc.address = (unsigned long)boot_idt;
> >
> >
> > - if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
> > + if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
> > + sev_snp_cpuid_init(rmode);
> > set_idt_entry(X86_TRAP_VC, boot_stage1_vc);
> > + }
> >
> > load_boot_idt(&boot_idt_desc);
> > }
> > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> > index 16b092fd7aa1..cdd328aa42c2 100644
> > --- a/arch/x86/boot/compressed/misc.h
> > +++ b/arch/x86/boot/compressed/misc.h
> > @@ -190,6 +190,7 @@ int efi_get_conf_table(struct boot_params *boot_params,
> > unsigned long *conf_table_pa,
> > unsigned int *conf_table_len,
> > bool *is_efi_64);
> > +
>
> Another stray hunk.
>
> > #else
> > static inline int
> > efi_find_vendor_table(unsigned long conf_table_pa, unsigned int conf_table_len,
> > diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> > index 6e8d97c280aa..910bf5cf010e 100644
> > --- a/arch/x86/boot/compressed/sev.c
> > +++ b/arch/x86/boot/compressed/sev.c
> > @@ -20,6 +20,9 @@
> > #include <asm/fpu/xcr.h>
> > #include <asm/ptrace.h>
> > #include <asm/svm.h>
> > +#include <asm/cpuid.h>
> > +#include <linux/efi.h>
> > +#include <linux/log2.h>
>
> What are those includes for?
>
> Polluting the decompressor namespace with kernel proper defines is a
> real pain to untangle as it is. What do you need those for and can you
> do it without them?
cpuid.h is for cpuid_function_is_indexed(), which was introduced in this
series with patch "KVM: x86: move lookup of indexed CPUID leafs to helper".
efi.h is for EFI_CC_BLOB_GUID, which gets referenced by sev-shared.c
when it gets included here. However, misc.h seems to already include it,
so it can be safely dropped from this patch.
log2.h seems to be an artifact, I'll get that cleaned up.
>
> > #include "error.h"
> >
> > diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> > index 072540dfb129..5f134c172dbf 100644
> > --- a/arch/x86/include/asm/sev-common.h
> > +++ b/arch/x86/include/asm/sev-common.h
> > @@ -148,6 +148,8 @@ struct snp_psc_desc {
> > #define GHCB_TERM_PSC 1 /* Page State Change failure */
> > #define GHCB_TERM_PVALIDATE 2 /* Pvalidate failure */
> > #define GHCB_TERM_NOT_VMPL0 3 /* SNP guest is not running at VMPL-0 */
> > +#define GHCB_TERM_CPUID 4 /* CPUID-validation failure */
> > +#define GHCB_TERM_CPUID_HV 5 /* CPUID failure during hypervisor fallback */
> >
> > #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK)
> >
> > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> > index 534fa1c4c881..c73931548346 100644
> > --- a/arch/x86/include/asm/sev.h
> > +++ b/arch/x86/include/asm/sev.h
> > @@ -11,6 +11,7 @@
> > #include <linux/types.h>
> > #include <asm/insn.h>
> > #include <asm/sev-common.h>
> > +#include <asm/bootparam.h>
> >
> > #define GHCB_PROTOCOL_MIN 1ULL
> > #define GHCB_PROTOCOL_MAX 2ULL
> > @@ -126,6 +127,7 @@ void __init snp_prep_memory(unsigned long paddr, unsigned int sz, enum psc_op op
> > void snp_set_memory_shared(unsigned long vaddr, unsigned int npages);
> > void snp_set_memory_private(unsigned long vaddr, unsigned int npages);
> > void snp_set_wakeup_secondary_cpu(void);
> > +void sev_snp_cpuid_init(struct boot_params *bp);
> > #else
> > static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> > static inline void sev_es_ist_exit(void) { }
> > @@ -141,6 +143,7 @@ static inline void __init snp_prep_memory(unsigned long paddr, unsigned int sz,
> > static inline void snp_set_memory_shared(unsigned long vaddr, unsigned int npages) { }
> > static inline void snp_set_memory_private(unsigned long vaddr, unsigned int npages) { }
> > static inline void snp_set_wakeup_secondary_cpu(void) { }
> > +static inline void sev_snp_cpuid_init(struct boot_params *bp) { }
> > #endif
> >
> > #endif
> > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> > index ae4556925485..651980ddbd65 100644
> > --- a/arch/x86/kernel/sev-shared.c
> > +++ b/arch/x86/kernel/sev-shared.c
> > @@ -14,6 +14,25 @@
> > #define has_cpuflag(f) boot_cpu_has(f)
> > #endif
> >
> > +struct sev_snp_cpuid_fn {
> > + u32 eax_in;
> > + u32 ecx_in;
> > + u64 unused;
> > + u64 unused2;
>
> What are those for? Padding? Or are they spec-ed somewhere and left for
> future use?
>
> Seeing how the struct is __packed, they probably are part of a spec
> definition somewhere.
>
> Link pls.
>
> > + u32 eax;
> > + u32 ebx;
> > + u32 ecx;
> > + u32 edx;
> > + u64 reserved;
>
> Ditto.
>
> Please prefix all those unused/reserved members with "__".
Will do.
>
> > +} __packed;
> > +
> > +struct sev_snp_cpuid_info {
> > + u32 count;
> > + u32 reserved1;
> > + u64 reserved2;
>
> Ditto.
The 'reserved' fields here are documented in SEV-SNP Firmware ABI
revision 0.9, section 8.14.2.6 (CPUID page), and the above 'reserved'
fields of sev_snp_cpuid_fn are documented in section 7.1 (CPUID Reporting)
Table 14:
https://www.amd.com/system/files/TechDocs/56860.pdf
The 'unused' / 'unused2' fields correspond to 'XCR0_IN' and 'XSS_IN' in
section 7.1 Table 14. They are meant to allow a hypervisor to encode
CPUID leaf 0xD subleaf 0x0:0x1 entries that are specific to a certain
set of XSAVE features enabled via XCR0/XSS registers, so a guest can
look up the specific entry based on its current XCR0/XSS register
values.
This doesn't scale very well as more XSAVE features are added however,
and was more useful for the CPUID guest message documented in 7.1, as
opposed to the static CPUID page implemented here.
Instead, it is simpler and just as safe to have the guest calculate the
appropriate values based on CPUID leaf 0xD, subleaves 0x2-0x3F, like
what sev_snp_cpuid_xsave_size() does below. So they are marked unused
here to try to make that clearer.
Some of these hypervisor-specific implementation notes have been summarized
into a document posted to the sev-snp mailing list in June:
"Guest/Hypervisor Implementation Notes for SEV-SNP CPUID Enforcement"
It's currently in RFC v2, but there has been a change relating to the
CPUID range checks that needs to be added for v3, I'll get that sent
out soon. We are hoping to get these included in an official spec to
help with interoperability between hypervisors, but for now it is only
a reference to aid implementations.
>
> > + struct sev_snp_cpuid_fn fn[0];
> > +} __packed;
> > +
> > /*
> > * Since feature negotiation related variables are set early in the boot
> > * process they must reside in the .data section so as not to be zeroed
> > @@ -26,6 +45,15 @@ static u16 __ro_after_init ghcb_version;
> > /* Bitmap of SEV features supported by the hypervisor */
> > u64 __ro_after_init sev_hv_features = 0;
> >
> > +/*
> > + * These are also stored in .data section to avoid the need to re-parse
> > + * boot_params and re-determine CPUID memory range when .bss is cleared.
> > + */
> > +static int sev_snp_cpuid_enabled __section(".data");
>
> That will become part of prot_guest_has() or cc_platform_has() or
> whatever its name is going to be.
Ok, will look at working this into there.
>
> > +static unsigned long sev_snp_cpuid_pa __section(".data");
> > +static unsigned long sev_snp_cpuid_sz __section(".data");
> > +static const struct sev_snp_cpuid_info *cpuid_info __section(".data");
>
> All those: __ro_after_init?
Makes sense.
>
> Also, just like the ones above have a short comment explaining what they
> are, add such comments for those too pls and perhaps what they're used
> for.
Will do.
>
> > +
> > static bool __init sev_es_check_cpu_features(void)
> > {
> > if (!has_cpuflag(X86_FEATURE_RDRAND)) {
> > @@ -236,6 +264,219 @@ static int sev_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> > return 0;
> > }
> >
> > +static bool sev_snp_cpuid_active(void)
> > +{
> > + return sev_snp_cpuid_enabled;
> > +}
>
> That too will become part of prot_guest_has() or cc_platform_has() or
> whatever its name is going to be.
>
> > +
> > +static int sev_snp_cpuid_xsave_size(u64 xfeatures_en, u32 base_size,
> > + u32 *xsave_size, bool compacted)
>
> Function name needs a verb. Please audit all your patches.
>
> > +{
> > + u64 xfeatures_found = 0;
> > + int i;
> > +
> > + *xsave_size = base_size;
>
> Set that xsave_size only...
> > +
> > + for (i = 0; i < cpuid_info->count; i++) {
> > + const struct sev_snp_cpuid_fn *fn = &cpuid_info->fn[i];
> > +
> > + if (!(fn->eax_in == 0xd && fn->ecx_in > 1 && fn->ecx_in < 64))
> > + continue;
> > + if (!(xfeatures_en & (1UL << fn->ecx_in)))
> > + continue;
> > + if (xfeatures_found & (1UL << fn->ecx_in))
> > + continue;
> > +
> > + xfeatures_found |= (1UL << fn->ecx_in);
>
> For all use BIT_ULL().
>
> > + if (compacted)
> > + *xsave_size += fn->eax;
> > + else
> > + *xsave_size = max(*xsave_size, fn->eax + fn->ebx);
>
> ... not here ...
>
> > + }
> > +
> > + /*
> > + * Either the guest set unsupported XCR0/XSS bits, or the corresponding
> > + * entries in the CPUID table were not present. This is not a valid
> > + * state to be in.
> > + */
> > + if (xfeatures_found != (xfeatures_en & ~3ULL))
> > + return -EINVAL;
>
> ... but here when you're not going to return an error because callers
> will see that value change temporarily which is not clean.
>
> Also, you need to set it once - not during each loop iteration.
Much nicer, will do.
>
> > +
> > + return 0;
> > +}
> > +
> > +static void sev_snp_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> > + u32 *ecx, u32 *edx)
> > +{
> > + /*
> > + * Currently MSR protocol is sufficient to handle fallback cases, but
> > + * should that change make sure we terminate rather than grabbing random
>
> Fix the "we"s please. Please audit all your patches.
>
> > + * values. Handling can be added in future to use GHCB-page protocol for
> > + * cases that occur late enough in boot that GHCB page is available
>
> End comment sentences with a fullstop. Please audit all your patches.
>
> > + */
>
> Also, put that comment over the function.
>
> > + if (cpuid_function_is_indexed(func) && subfunc != 0)
>
> In all your patches:
>
> s/ != 0//g
>
> > + sev_es_terminate(1, GHCB_TERM_CPUID_HV);
> > +
> > + if (sev_cpuid_hv(func, 0, eax, ebx, ecx, edx))
> > + sev_es_terminate(1, GHCB_TERM_CPUID_HV);
> > +}
> > +
> > +static bool sev_snp_cpuid_find(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
>
> I guess
>
> find_validated_cpuid_func()
>
> or so to denote where it picks it out from.
>
> > + u32 *ecx, u32 *edx)
> > +{
> > + int i;
> > + bool found = false;
>
> The tip-tree preferred ordering of variable declarations at the
> beginning of a function is reverse fir tree order::
>
> struct long_struct_name *descriptive_name;
> unsigned long foo, bar;
> unsigned int tmp;
> int ret;
>
> The above is faster to parse than the reverse ordering::
>
> int ret;
> unsigned int tmp;
> unsigned long foo, bar;
> struct long_struct_name *descriptive_name;
>
> And even more so than random ordering::
>
> unsigned long foo, bar;
> int ret;
> struct long_struct_name *descriptive_name;
> unsigned int tmp;
>
> Audit all your patches pls.
>
> > +
> > + for (i = 0; i < cpuid_info->count; i++) {
> > + const struct sev_snp_cpuid_fn *fn = &cpuid_info->fn[i];
> > +
> > + if (fn->eax_in != func)
> > + continue;
> > +
> > + if (cpuid_function_is_indexed(func) && fn->ecx_in != subfunc)
> > + continue;
> > +
> > + *eax = fn->eax;
> > + *ebx = fn->ebx;
> > + *ecx = fn->ecx;
> > + *edx = fn->edx;
> > + found = true;
> > +
> > + break;
>
> That's just silly. Simply:
>
> return true;
>
>
> > + }
> > +
> > + return found;
>
> return false;
>
> here and the "found" variable can go.
Will do. Missed this cleanup when I originally moved this out to a
seperate helper.
>
> > +}
> > +
> > +static bool sev_snp_cpuid_in_range(u32 func)
> > +{
> > + int i;
> > + u32 std_range_min = 0;
> > + u32 std_range_max = 0;
> > + u32 hyp_range_min = 0x40000000;
> > + u32 hyp_range_max = 0;
> > + u32 ext_range_min = 0x80000000;
> > + u32 ext_range_max = 0;
> > +
> > + for (i = 0; i < cpuid_info->count; i++) {
> > + const struct sev_snp_cpuid_fn *fn = &cpuid_info->fn[i];
> > +
> > + if (fn->eax_in == std_range_min)
> > + std_range_max = fn->eax;
> > + else if (fn->eax_in == hyp_range_min)
> > + hyp_range_max = fn->eax;
> > + else if (fn->eax_in == ext_range_min)
> > + ext_range_max = fn->eax;
> > + }
>
> So this loop which determines those ranges will run each time
> sev_snp_cpuid_find() doesn't find @func among the validated CPUID leafs.
>
> Why don't you do that determination once at init...
>
> > +
> > + if ((func >= std_range_min && func <= std_range_max) ||
> > + (func >= hyp_range_min && func <= hyp_range_max) ||
> > + (func >= ext_range_min && func <= ext_range_max))
>
> ... so that this function becomes only this check?
>
> This is unnecessary work as it is.
That makes sense. I was treating this as an edge case but it could actually
happen fairly often in some case. I'll plan to add __ro_after_init
variables to store these values.
>
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +/*
> > + * Returns -EOPNOTSUPP if feature not enabled. Any other return value should be
> > + * treated as fatal by caller since we cannot fall back to hypervisor to fetch
> > + * the values for security reasons (outside of the specific cases handled here)
> > + */
> > +static int sev_snp_cpuid(u32 func, u32 subfunc, u32 *eax, u32 *ebx, u32 *ecx,
> > + u32 *edx)
> > +{
> > + if (!sev_snp_cpuid_active())
> > + return -EOPNOTSUPP;
> > +
> > + if (!cpuid_info)
> > + return -EIO;
> > +
> > + if (!sev_snp_cpuid_find(func, subfunc, eax, ebx, ecx, edx)) {
> > + /*
> > + * Some hypervisors will avoid keeping track of CPUID entries
> > + * where all values are zero, since they can be handled the
> > + * same as out-of-range values (all-zero). In our case, we want
> > + * to be able to distinguish between out-of-range entries and
> > + * in-range zero entries, since the CPUID table entries are
> > + * only a template that may need to be augmented with
> > + * additional values for things like CPU-specific information.
> > + * So if it's not in the table, but is still in the valid
> > + * range, proceed with the fix-ups below. Otherwise, just return
> > + * zeros.
> > + */
> > + *eax = *ebx = *ecx = *edx = 0;
> > + if (!sev_snp_cpuid_in_range(func))
> > + goto out;
>
> That label is not needed.
>
> > + }
>
> All that from here on looks like it should go into a separate function
> called
>
> snp_cpuid_postprocess()
>
> where you can do a switch-case on func and have it nice, readable and
> extensible there, in case more functions get added.
Sounds good.
> > /*
> > * 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
>
> Is that comment...
Technically it supports MSR communication *and* CPUID page lookups now.
Assuming that's what you're referring to I'll get that added.
>
> > @@ -244,15 +485,25 @@ static int sev_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> > void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
> > {
> > unsigned int fn = lower_bits(regs->ax, 32);
> > + unsigned int subfn = lower_bits(regs->cx, 32);
> > u32 eax, ebx, ecx, edx;
> > + int ret;
> >
> > /* Only CPUID is supported via MSR protocol */
>
> ... and that still valid?
"Only CPUID #VCs can be handled without using a GHCB page" might be a
bit more to the point now. I'll update it.
> > +
> > +out_verify:
> > + /* CC blob should be either valid or not present. Fail otherwise. */
> > + if (cc_info && cc_info->magic != CC_BLOB_SEV_HDR_MAGIC)
> > + sev_es_terminate(1, GHCB_SNP_UNSUPPORTED);
> > +
> > + return cc_info;
> > +}
> > +#else
> > +/*
> > + * Probing for CC blob for run-time kernel will be enabled in a subsequent
> > + * patch. For now we need to stub this out.
> > + */
> > +static struct cc_blob_sev_info *sev_snp_probe_cc_blob(struct boot_params *bp)
> > +{
> > + return NULL;
> > +}
> > +#endif
> > +
> > +/*
> > + * Initial set up of CPUID table when running identity-mapped.
> > + *
> > + * NOTE: Since SEV_SNP feature partly relies on CPUID checks that can't
> > + * happen until we access CPUID page, we skip the check and hope the
> > + * bootloader is providing sane values.
>
> So I don't like the sound of that even one bit. We shouldn't hope
> anything here...
More specifically, the general protocol to determine SNP is enabled seems
to be:
1) check cpuid 0x8000001f to determine if SEV bit is enabled and SEV
MSR is available
2) check the SEV MSR to see if SEV-SNP bit is set
but the conundrum here is the CPUID page is only valid if SNP is
enabled, otherwise it can be garbage. So the code to set up the page
skips those checks initially, and relies on the expectation that UEFI,
or whatever the initial guest blob was, will only provide a CC_BLOB if
it already determined SNP is enabled.
It's still possible something goes awry and the kernel gets handed a
bogus CC_BLOB even though SNP isn't actually enabled. In this case the
cpuid values could be bogus as well, but the guest will fail
attestation then and no secrets should be exposed.
There is one thing that could tighten up the check a bit though. Some
bits of SEV-ES code will use the generation of a #VC as an indicator
of SEV-ES support, which implies SEV MSR is available without relying
on hypervisor-provided CPUID bits. I could add a one-time check in
the cpuid #VC to check SEV MSR for SNP bit, but it would likely
involve another static __ro_after_init variable store state. If that
seems worthwhile I can look into that more as well.
>
> > Current code relies on all CPUID
> > + * page lookups originating from #VC handler, which at least provides
> > + * indication that SEV-ES is enabled. Subsequent init levels will check for
> > + * SEV_SNP feature once available to also take SEV MSR value into account.
> > + */
> > +void sev_snp_cpuid_init(struct boot_params *bp)
>
> snp_cpuid_init()
>
> In general, prefix all SNP-specific variables, structs, functions, etc
> with "snp_" simply.
>
> > +{
> > + struct cc_blob_sev_info *cc_info;
> > +
> > + if (!bp)
> > + sev_es_terminate(1, GHCB_TERM_CPUID);
> > +
> > + cc_info = sev_snp_probe_cc_blob(bp);
> > +
>
> ^ Superfluous newline.
>
> > + if (!cc_info)
> > + return;
> > +
> > + sev_snp_cpuid_pa = cc_info->cpuid_phys;
> > + sev_snp_cpuid_sz = cc_info->cpuid_len;
>
> You can do those assignments ...
>
> > +
> > + /*
> > + * These should always be valid values for SNP, even if guest isn't
> > + * actually configured to use the CPUID table.
> > + */
> > + if (!sev_snp_cpuid_pa || sev_snp_cpuid_sz < PAGE_SIZE)
> > + sev_es_terminate(1, GHCB_TERM_CPUID);
>
>
> ... here, after you've verified them.
>
> > +
> > + cpuid_info = (const struct sev_snp_cpuid_info *)sev_snp_cpuid_pa;
> > +
> > + /*
> > + * We should be able to trust the 'count' value in the CPUID table
> > + * area, but ensure it agrees with CC blob value to be safe.
> > + */
> > + if (sev_snp_cpuid_sz < (sizeof(struct sev_snp_cpuid_info) +
> > + sizeof(struct sev_snp_cpuid_fn) *
> > + cpuid_info->count))
>
> Yah, this is the type of paranoia I'm talking about!
>
> > + sev_es_terminate(1, GHCB_TERM_CPUID);
> > +
> > + sev_snp_cpuid_enabled = 1;
> > +}
> > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> > index ddf8ced4a879..d7b6f7420551 100644
> > --- a/arch/x86/kernel/sev.c
> > +++ b/arch/x86/kernel/sev.c
> > @@ -19,6 +19,8 @@
> > #include <linux/kernel.h>
> > #include <linux/mm.h>
> > #include <linux/cpumask.h>
> > +#include <linux/log2.h>
> > +#include <linux/efi.h>
> >
> > #include <asm/cpu_entry_area.h>
> > #include <asm/stacktrace.h>
> > @@ -32,6 +34,8 @@
> > #include <asm/smp.h>
> > #include <asm/cpu.h>
> > #include <asm/apic.h>
> > +#include <asm/efi.h>
> > +#include <asm/cpuid.h>
> >
> > #include "sev-internal.h"
>
> What are those includes for?
These are also for EFI_CC_BLOB_GUID and cpuid_function_is_indexed,
respectively. Will add comments to clarify.
Thanks for the thorough review, will address all comments.
>
> Looks like a leftover...
>
> --
> 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%7C4640a85832a9482de34f08d967fd2972%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637655159332919463%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=jpLwHsCIsXMV1jvwBp7DjPX4RAnw5tWRqB%2F9Ddccp%2FY%3D&reserved=0
Powered by blists - more mailing lists