Re: [PATCH v9 38/43] x86/sev: Use firmware-validated CPUID for SEV-SNP guests

[Michael Roth <michael.roth@amd.com>]

On Fri, Jan 28, 2022 at 11:17:59AM -0600, Brijesh Singh wrote:
> From: Michael Roth <michael.roth@amd.com>
> 
> SEV-SNP guests will be provided the location of special 'secrets' and
> 'CPUID' pages via the Confidential Computing blob. This blob is
> provided to the run-time kernel either through a bootparams field that
> was initialized by the boot/compressed kernel, or via a setup_data
> structure as defined by the Linux Boot Protocol.
> 
> Locate the Confidential Computing blob from these sources and, if found,
> use the provided CPUID page/table address to create a copy that the
> run-time kernel will use when servicing CPUID instructions via a #VC
> handler.
> 
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/boot/compressed/sev.c | 37 -----------------
>  arch/x86/kernel/sev-shared.c   | 37 +++++++++++++++++
>  arch/x86/kernel/sev.c          | 75 ++++++++++++++++++++++++++++++++++
>  3 files changed, 112 insertions(+), 37 deletions(-)
> 
<snip>
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 5c72b21c7e7b..cb97200bfda7 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -2034,6 +2034,8 @@ bool __init snp_init(struct boot_params *bp)
>  	if (!cc_info)
>  		return false;
>  
> +	snp_setup_cpuid_table(cc_info);
> +
>  	/*
>  	 * The CC blob will be used later to access the secrets page. Cache
>  	 * it here like the boot kernel does.
> @@ -2047,3 +2049,76 @@ void __init snp_abort(void)
>  {
>  	sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>  }
> +
> +static void snp_dump_cpuid_table(void)
> +{
> +	const struct snp_cpuid_info *cpuid_info = snp_cpuid_info_get_ptr();
> +	int i = 0;
> +
> +	pr_info("count=%d reserved=0x%x reserved2=0x%llx\n",
> +		cpuid_info->count, cpuid_info->__reserved1, cpuid_info->__reserved2);
> +
> +	for (i = 0; i < SNP_CPUID_COUNT_MAX; i++) {
> +		const struct snp_cpuid_fn *fn = &cpuid_info->fn[i];
> +
> +		pr_info("index=%03d fn=0x%08x subfn=0x%08x: eax=0x%08x ebx=0x%08x ecx=0x%08x edx=0x%08x xcr0_in=0x%016llx xss_in=0x%016llx reserved=0x%016llx\n",
> +			i, fn->eax_in, fn->ecx_in, fn->eax, fn->ebx, fn->ecx,
> +			fn->edx, fn->xcr0_in, fn->xss_in, fn->__reserved);
> +	}
> +}
> +
> +/*
> + * It is useful from an auditing/testing perspective to provide an easy way
> + * for the guest owner to know that the CPUID table has been initialized as
> + * expected, but that initialization happens too early in boot to print any
> + * sort of indicator, and there's not really any other good place to do it.
> + * So do it here. This is also a good place to flag unexpected usage of the
> + * CPUID table that does not violate the SNP specification, but may still
> + * be worth noting to the user.
> + */
> +static int __init snp_check_cpuid_table(void)
> +{
> +	const struct snp_cpuid_info *cpuid_info = snp_cpuid_info_get_ptr();
> +	bool dump_table = false;
> +	int i;
> +
> +	if (!cpuid_info->count)
> +		return 0;
> +
> +	pr_info("Using SEV-SNP CPUID table, %d entries present.\n",
> +		cpuid_info->count);
> +
> +	if (cpuid_info->__reserved1 || cpuid_info->__reserved2) {
> +		pr_warn("Unexpected use of reserved fields in CPUID table header.\n");
> +		dump_table = true;
> +	}
> +
> +	for (i = 0; i < cpuid_info->count; i++) {
> +		const struct snp_cpuid_fn *fn = &cpuid_info->fn[i];
> +		struct snp_cpuid_fn zero_fn = {0};
> +
> +		/*
> +		 * Though allowed, an all-zero entry is not a valid leaf for linux
> +		 * guests, and likely the result of an incorrect entry count.
> +		 */
> +		if (!memcmp(fn, &zero_fn, sizeof(struct snp_cpuid_fn))) {
> +			pr_warn("CPUID table contains all-zero entry at index=%d\n", i);
> +			dump_table = true;
> +		}
> +
> +		if (fn->__reserved) {
> +			pr_warn("Unexpected use of reserved fields in CPUID table entry at index=%d\n",
> +				i);
> +			dump_table = true;
> +		}
> +	}
> +
> +	if (dump_table) {
> +		pr_warn("Dumping CPUID table:\n");
> +		snp_dump_cpuid_table();
> +	}
> +
> +	return 0;
> +}
> +
> +arch_initcall(snp_check_cpuid_table);

Just a quick note on this one. I know you'd recently suggested dropping
this check since it was redundant:

  https://lore.kernel.org/all/YfGUWLmg82G+l4jU@zn.tnic/

I've dropped the redundant checks from in this version, but with the
additional sanity checks you suggested adding, this function remained
relevant for v9 so we can provide user-readable warnings for "weird"
stuff in the table that doesn't necessarily violate the spec, but also
doesn't seem to make much sense:

  https://lore.kernel.org/all/YefzQuqrV8kdLr9z@zn.tnic/

But I also agree with your later response here, where sanity-checking
doesn't gain us much since ultimately what really matter is the
resulting values we provide in response to CPUID instructions, which
would be ultimately what gets used for guest owner's attestation flow:

  https://lore.kernel.org/all/YfR1GNb%2FyzKu4n5+@zn.tnic/

So I agree we should drop the sanity-check in this function since
there's no cases covered here that would actually effect the end
result of those CPUID instructions, nor do any of the checks here
violate the SNP spec...

...except possibly the checks that enforce that the Reserved fields
here are all 0, which we discussed here:

  https://lore.kernel.org/all/YeGhKll2fTcTr2wS@zn.tnic/

I followed up with our firmware team on this to confirm whether our
intended handling and the potential backward-compatibility issues
aligned with the intent of the SNP FW spec, and the guidance I got
on that is that those Reserved/MBZ constraints are meant to apply
to what a hypervisor places in the initial CPUID table input to
SNP_LAUNCH_UPDATE (or what the guest places in the MSG_CPUID_REQ
structure in the guest message version). They are not meant to
cover what the guest should expect to read for these Reserved fields
in the resulting CPUID table: the guest is supposed to ignore them
completely and not enforce any value.

I mentioned the concern you raised about out-of-spec hypervisors
using non-zero for Reserved fields, and potentially breaking future
guests that attempt to make use of them if they ever get re-purposed
for some other functionality, but their take on that is that such a
hypervisor is clearly out-of-spec, and would not be supported. Their
preference would be to update the spec for clarity on this if we
have any better suggested wording, rather than implementing anything
on the guest side that might effect backward-compatibility in the
future. Another possibility is enforcing 0 in the firmware itself.

So given their guidance on the Reserved fields, and your guidance
on not doing the other sanity-checks, my current plan to to drop
snp_check_cpuid_table() completely for v10, but if you have other
thoughts on that just let me know.

Thanks!

-Mike


> -- 
> 2.25.1
>