| Date | Mon, 7 Feb 2022 09:52:09 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH v9 42/43] virt: sevguest: Add support to derive key |
| |
On Fri, Jan 28, 2022 at 11:18:03AM -0600, Brijesh Singh wrote: > +static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) > +{ > + struct snp_guest_crypto *crypto = snp_dev->crypto; > + struct snp_derived_key_resp resp = {0}; > + struct snp_derived_key_req req = {0}; > + int rc, resp_len; > + u8 buf[64+16]; /* Response data is 64 bytes and max authsize for GCM is 16 bytes */
verify_comment_style: Warning: No tail comments please: drivers/virt/coco/sevguest/sevguest.c:401 [+ u8 buf[64+16]; /* Response data is 64 bytes and max authsize for GCM is 16 bytes */]
> + if (!arg->req_data || !arg->resp_data) > + return -EINVAL; > + > + /* Copy the request payload from userspace */
That comment looks useless.
> + if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req))) > + return -EFAULT; > + > + /* > + * 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; > + if (sizeof(buf) < resp_len) > + return -ENOMEM;
That test can happen before the copy_from_user() above.
> + > + /* Issue the command to get the attestation report */
Also useless.
> + rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg->msg_version, > + SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len, > + &arg->fw_err); > + if (rc) > + goto e_free; > + > + /* Copy the response payload to userspace */
Ditto.
> + memcpy(resp.data, buf, sizeof(resp.data)); > + if (copy_to_user((void __user *)arg->resp_data, &resp, sizeof(resp))) > + rc = -EFAULT; > + > +e_free: > + memzero_explicit(buf, sizeof(buf)); > + memzero_explicit(&resp, sizeof(resp));
Those are allocated on stack, why are you clearing them?
> + return rc; > +}
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
|