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

From: Brijesh Singh
Date: Wed Sep 08 2021 - 17:44:45 EST

On 9/8/21 9:00 AM, Borislav Petkov wrote:
On Fri, Aug 20, 2021 at 10:19:32AM -0500, Brijesh Singh wrote:
+: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 will contains the derived key

"will contain"


+ /* Copy the request payload from the userspace */

"from userspace"


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);
+ 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.

I will look into improving it to copy back to userspace only if there is firmware error.