Re: [PATCH v11 42/45] virt: Add SEV-SNP guest driver

Dave Hansen
2022-03-03T17:34:02Z

> +++ b/drivers/virt/coco/sevguest/Kconfig
> @@ -0,0 +1,12 @@
> +config SEV_GUEST
> + tristate "AMD SEV Guest driver"
> + default m
> + depends on AMD_MEM_ENCRYPT && CRYPTO_AEAD2
> + help
> + SEV-SNP firmware provides the guest a mechanism to communicate with
> + the PSP without risk from a malicious hypervisor who wishes to read,
> + alter, drop or replay the messages sent. The driver provides
> + userspace interface to communicate with the PSP to request the
> + attestation report and more.
> +
> + If you choose 'M' here, this module will be called sevguest.
...
> +/* Return a non-zero on success */
> +static u64 snp_get_msg_seqno(struct snp_guest_dev *snp_dev)
> +{
> + u64 count = __snp_get_msg_seqno(snp_dev);
> +
> + /*
> + * The message sequence counter for the SNP guest request is a 64-bit
> + * value but the version 2 of GHCB specification defines a 32-bit storage
> + * for it. If the counter exceeds the 32-bit value then return zero.
> + * The caller should check the return value, but if the caller happens to
> + * not check the value and use it, then the firmware treats zero as an
> + * invalid number and will fail the message request.
> + */
> + if (count >= UINT_MAX) {
> + pr_err_ratelimited("request message sequence counter overflow\n");
> + return 0;
> + }
> +
> + return count;
> +}

I didn't see a pr_fmt defined anywhere. But, for a "driver", should
this be a dev_err()?

...
> +static void free_shared_pages(void *buf, size_t sz)
> +{
> + unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +
> + if (!buf)
> + return;
> +
> + if (WARN_ONCE(set_memory_encrypted((unsigned long)buf, npages),
> + "failed to restore encryption mask (leak it)\n"))
> + return;
> +
> + __free_pages(virt_to_page(buf), get_order(sz));
> +}

Nit: It's a bad practice to do important things inside a WARN_ON() _or_
and if(). This should be:

int ret;

...

ret = set_memory_encrypted((unsigned long)buf, npages));

if (ret) {
WARN_ONCE(...);
return;
}

BTW, this look like a generic allocator thingy. But it's only ever used
to allocate a 'struct snp_guest_msg'. Why all the trouble to allocate
and free one fixed-size structure? The changelog and comments don't
shed any light.