Re: [PATCH v7 44/45] virt: sevguest: Add support to derive key

Brijesh Singh
2021-11-18T17:43:31Z

On 11/18/21 10:43 AM, Peter Gonda wrote:
...
>> + u8 buf[89];
>
> Could we document this magic number?
>

Yes, I will document from where this number came.

>> +
>> + if (!arg->req_data || !arg->resp_data)
>> + return -EINVAL;
>> +
>> + /* Copy the request payload from userspace */
>> + if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
>> + return -EFAULT;
>> +
>> + /* Message version must be non-zero */
>> + if (!req.msg_version)
>> + return -EINVAL;
>> +
>> + /*
>> + * 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;
>> +
>> + /* Issue the command to get the attestation report */
>> + rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, req.msg_version,
>> + SNP_MSG_KEY_REQ, &req.data, sizeof(req.data), buf, resp_len,
>> + &arg->fw_err);
>> + if (rc)
>> + goto e_free;
>
> Should we check the first 32 bits of |data| here since that is a
> status field? If we see 16h here we could return -EINVAL, or better to
> let userspace deal with that error handling?
>

I was trying to avoid looking into a response structure to keep the
flexibility; e.g if SNP firmware changes a response format then we don't
need to have any changes in the driver. The userspace should be able to
deal with it and it can check the "status" or a new field etc.

thanks