On 01/09/2021 0:04, Brijesh Singh wrote: > 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 ? > Yes, that's the concern, specifically with sensitive buffers (keys). You don't want many copies floating around in unused memory. -Dov
