Re: [PATCH v11 44/45] virt: sevguest: Add support to get extended report

Borislav Petkov
2022-03-03T15:28:17Z

On Thu, Feb 24, 2022 at 10:56:24AM -0600, Brijesh Singh wrote:
> +static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
> +{
> + struct snp_guest_crypto *crypto = snp_dev->crypto;
> + struct snp_ext_report_req req = {0};
> + struct snp_report_resp *resp;
> + int ret, npages = 0, resp_len;
> +
> + lockdep_assert_held(&snp_cmd_mutex);
> +
> + if (!arg->req_data || !arg->resp_data)
> + return -EINVAL;
> +
> + if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> + return -EFAULT;
> +
> + if (req.certs_len) {
> + if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
> + !IS_ALIGNED(req.certs_len, PAGE_SIZE))
> + return -EINVAL;
> + }
> +
> + if (req.certs_address && req.certs_len) {
> + if (!access_ok(req.certs_address, req.certs_len))
> + return -EFAULT;
> +
> + /*
> + * Initialize the intermediate buffer with all zeros. This buffer
> + * is used in the guest request message to get the certs blob from
> + * the host. If host does not supply any certs in it, then copy
> + * zeros to indicate that certificate data was not provided.
> + */
> + memset(snp_dev->certs_data, 0, req.certs_len);
> +
> + npages = req.certs_len >> PAGE_SHIFT;
> + }

I think all those checks should be made more explicit. This makes the
code a lot more readable and straight-forward (pasting the full excerpt
because the incremental diff ontop is less readable):

...

if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
return -EFAULT;

if (!req.certs_len || !req.certs_address)
return -EINVAL;

if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
!IS_ALIGNED(req.certs_len, PAGE_SIZE))
return -EINVAL;

if (!access_ok(req.certs_address, req.certs_len))
return -EFAULT;

/*
* Initialize the intermediate buffer with all zeros. This buffer
* is used in the guest request message to get the certs blob from
* the host. If host does not supply any certs in it, then copy
* zeros to indicate that certificate data was not provided.
*/
memset(snp_dev->certs_data, 0, req.certs_len);

npages = req.certs_len >> PAGE_SHIFT;

--
Regards/Gruss,
Boris.