Messages in this thread | | | Subject | Re: [PATCH Part1 v5 37/38] virt: sevguest: Add support to derive key | From | Brijesh Singh <> | Date | Tue, 31 Aug 2021 16:04:12 -0500 |
| |
Hi Dov,
On 8/31/21 1:59 PM, Dov Murik wrote: >> + >> + /* >> + * 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); > > The length of resp->data is 64 bytes; I assume crypto->a_len is not a > lot more (and probably known in advance for AES GCM). Maybe use a > buffer on the stack instead of allocating and freeing? >
The authtag size can be up to 16 bytes, so I guess I can allocate 80 bytes on stack and avoid the kzalloc().
> >> + 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); > > Since resp contains key material, I think you should explicit_memzero() > it before freeing, so the key bytes don't linger around in unused > memory. I'm not sure if any copies are made inside the > handle_guest_request call above; maybe zero these as well. >
I can do that, but I guess I am trying to find a reason for it. The resp buffer is encrypted page, so, the key is protected from the hypervisor access. Are you thinking about an attack within the VM guest OS ?
-Brijesh
| |