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

[Borislav Petkov]

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.

https://people.kernel.org/tglx/notes-about-netiquette