Re: [PATCH Part1 v5 37/38] virt: sevguest: Add support to derive key

[Borislav Petkov]

On Fri, Aug 20, 2021 at 10:19:32AM -0500, Brijesh Singh wrote:
> +2.2 SNP_GET_DERIVED_KEY
> +-----------------------
> +:Technology: sev-snp
> +:Type: guest ioctl
> +:Parameters (in): struct snp_derived_key_req
> +:Returns (out): struct snp_derived_key_req on success, -negative on error
> +
> +The SNP_GET_DERIVED_KEY ioctl can be used to get a key derive from a root key.
> +The derived key can be used by the guest for any purpose, such as sealing keys
> +or communicating with external entities.
> +
> +The ioctl uses the SNP_GUEST_REQUEST (MSG_KEY_REQ) command provided by the
> +SEV-SNP firmware to derive the key. See SEV-SNP specification for further details
> +on the various fileds passed in the key derivation request.
> +
> +On success, the snp_derived_key_resp.data will contains the derived key

"will contain"

> +value.
> diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c
> index d029a98ad088..621b1c5a9cfc 100644
> --- a/drivers/virt/coco/sevguest/sevguest.c
> +++ b/drivers/virt/coco/sevguest/sevguest.c
> @@ -303,6 +303,50 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_user_guest_reque
>  	return rc;
>  }
>  
> +static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *arg)
> +{
> +	struct snp_guest_crypto *crypto = snp_dev->crypto;
> +	struct snp_derived_key_resp *resp;
> +	struct snp_derived_key_req req;
> +	int rc, resp_len;
> +
> +	if (!arg->req_data || !arg->resp_data)
> +		return -EINVAL;
> +
> +	/* Copy the request payload from the userspace */

"from userspace"

> +	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> +		return -EFAULT;
> +
> +	/* Message version must be non-zero */
> +	if (!req.msg_version)
> +		return -EINVAL;
> +
> +	/*
> +	 * The intermediate response buffer is used while decrypting the
> +	 * response payload. Make sure that it has enough space to cover the
> +	 * authtag.
> +	 */
> +	resp_len = sizeof(resp->data) + crypto->a_len;
> +	resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT);
> +	if (!resp)
> +		return -ENOMEM;
> +
> +	/* Issue the command to get the attestation report */
> +	rc = handle_guest_request(snp_dev, req.msg_version, SNP_MSG_KEY_REQ,
> +				  &req.data, sizeof(req.data), resp->data, resp_len,
> +				  &arg->fw_err);
> +	if (rc)
> +		goto e_free;
> +
> +	/* Copy the response payload to userspace */
> +	if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
> +		rc = -EFAULT;
> +
> +e_free:
> +	kfree(resp);
> +	return rc;
> +}
> +
>  static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>  {
>  	struct snp_guest_dev *snp_dev = to_snp_dev(file);
> @@ -320,6 +364,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
>  		ret = get_report(snp_dev, &input);
>  		break;
>  	}
> +	case SNP_GET_DERIVED_KEY: {
> +		ret = get_derived_key(snp_dev, &input);
> +		break;
> +	}

{} brackets are not needed.

What, however, is bothering me more in this function is that you call
the respective ioctl function which might fail, you do not look at the
return value and copy_to_user() unconditionally.

Looking at get_derived_key(), for example, if it returns after:

        if (!arg->req_data || !arg->resp_data)
                return -EINVAL;

you will be copying the same thing back to the user, you copied in
earlier. That doesn't make any sense to me.

Thx.

-- 
Regards/Gruss,
    Boris.

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