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
