Re: [PATCH Part1 RFC v3 21/22] x86/sev: Register SNP guest request platform device

[Brijesh Singh <brijesh.singh@amd.com>]

I see that Tom answered few comments. I will cover others.


On 6/9/21 2:24 PM, Dr. David Alan Gilbert wrote:
+ /*
>> +	 * The message sequence counter for the SNP guest request is a 64-bit value
>> +	 * but the version 2 of GHCB specification defines the 32-bit storage for the
>> +	 * it.
>> +	 */
>> +	if ((count + 1) >= INT_MAX)
>> +		return 0;
> Is that UINT_MAX?

Good catch. It should be UINT_MAX.


> +	/*
> +	 * The secret page contains the VM encryption key used for encrypting the
> +	 * messages between the guest and the PSP. The secrets page location is
> +	 * available either through the setup_data or EFI configuration table.
> +	 */
> +	if (hdr->cc_blob_address) {
> +		paddr = hdr->cc_blob_address;
> Can you trust the paddr the host has given you or do you need to do some
> form of validation?
The paddr is mapped encrypted. That means that data  in the paddr must
be encrypted either through the guest or PSP. After locating the paddr,
we perform a simply sanity check (32-bit magic string "AMDE"). See the
verify header check below. Unfortunately the secrets page itself does
not contain any magic key which we can use to ensure that
hdr->secret_paddr is actually pointing to the secrets pages but all of
these memory is accessed encrypted so its safe to access it. If VMM
lying to us that basically means guest will not be able to communicate
with the PSP and can't do the attestation etc.

>
> Dave
> +	} else if (efi_enabled(EFI_CONFIG_TABLES)) {
> +#ifdef CONFIG_EFI
> +		paddr = cc_blob_phys;
> +#else
> +		return -ENODEV;
> +#endif
> +	} else {
> +		return -ENODEV;
> +	}
> +
> +	info = memremap(paddr, sizeof(*info), MEMREMAP_WB);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	/* Verify the header that its a valid SEV_SNP CC header */
> +	if ((info->magic == CC_BLOB_SEV_HDR_MAGIC) &&
> +	    info->secrets_phys &&
> +	    (info->secrets_len == PAGE_SIZE)) {
> +		res->start = info->secrets_phys;
> +		res->end = info->secrets_phys + info->secrets_len;
> +		res->flags = IORESOURCE_MEM;
> +		snp_secrets_phys = info->secrets_phys;
> +		ret = 0;
> +	}
> +
> +	memunmap(info);
> +	return ret;
> +}
> +