by Brijesh Singh

[permalink] [raw]

Subject: Re: [PATCH Part1 v5 38/38] virt: sevguest: Add support to get extended report

On 9/15/21 5:02 AM, Dr. David Alan Gilbert wrote:
> * Brijesh Singh ([email protected]) wrote:
>> Version 2 of GHCB specification defines NAE to get the extended guest
>> request. It is similar to the SNP_GET_REPORT ioctl. The main difference
> ^^^^^^^^^ is that 'report' not request?
>
>> is related to the additional data that be returned. The additional
>> data returned is a certificate blob that can be used by the SNP guest
>> user. The certificate blob layout is defined in the GHCB specification.
>> The driver simply treats the blob as a opaque data and copies it to
>> userspace.
>>
>> Signed-off-by: Brijesh Singh <[email protected]>
> I'm confused by snp_dev->certs_data - who writes to that, and when?
> I see it's allocated as shared by the probe function but then passed in
> input data in get_ext_report - but get_ext_report memset's it.
> What happens if two threads were to try and get an extended report at
> the same time?
The certs are system wide and is programmed by the Hypervisor during the
platform provisioning.The hypervisor copies the cert blob in the guest
memory while responding to the extended guest message request vmgexit.
The call to the guest message request function is serialized. i.e there
is a mutex_lock() before the get_ext_report().

> Dave
>
>
>> ---
>> Documentation/virt/coco/sevguest.rst | 22 +++++
>> drivers/virt/coco/sevguest/sevguest.c | 126 ++++++++++++++++++++++++++
>> include/uapi/linux/sev-guest.h | 13 +++
>> 3 files changed, 161 insertions(+)
>>
>> diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst
>> index 25446670d816..7acb8696fca4 100644
>> --- a/Documentation/virt/coco/sevguest.rst
>> +++ b/Documentation/virt/coco/sevguest.rst
>> @@ -85,3 +85,25 @@ on the various fileds passed in the key derivation request.
>>
>> On success, the snp_derived_key_resp.data will contains the derived key
>> value.
>> +
>> +2.2 SNP_GET_EXT_REPORT
>> +----------------------
>> +:Technology: sev-snp
>> +:Type: guest ioctl
>> +:Parameters (in/out): struct snp_ext_report_req
>> +:Returns (out): struct snp_report_resp on success, -negative on error
>> +
>> +The SNP_GET_EXT_REPORT ioctl is similar to the SNP_GET_REPORT. The difference is
>> +related to the additional certificate data that is returned with the report.
>> +The certificate data returned is being provided by the hypervisor through the
>> +SNP_SET_EXT_CONFIG.
>> +
>> +The ioctl uses the SNP_GUEST_REQUEST (MSG_REPORT_REQ) command provided by the SEV-SNP
>> +firmware to get the attestation report.
>> +
>> +On success, the snp_ext_report_resp.data will contains the attestation report
>> +and snp_ext_report_req.certs_address will contains the certificate blob. If the
>> +length of the blob is lesser than expected then snp_ext_report_req.certs_len will
>> +be updated with the expected value.
>> +
>> +See GHCB specification for further detail on how to parse the certificate blob.
>> diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c
>> index 621b1c5a9cfc..d978eb432c4c 100644
>> --- a/drivers/virt/coco/sevguest/sevguest.c
>> +++ b/drivers/virt/coco/sevguest/sevguest.c
>> @@ -39,6 +39,7 @@ struct snp_guest_dev {
>> struct device *dev;
>> struct miscdevice misc;
>>
>> + void *certs_data;
>> struct snp_guest_crypto *crypto;
>> struct snp_guest_msg *request, *response;
>> };
>> @@ -347,6 +348,117 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_user_guest_
>> return rc;
>> }
>>
>> +static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *arg)
>> +{
>> + struct snp_guest_crypto *crypto = snp_dev->crypto;
>> + struct snp_guest_request_data input = {};
>> + struct snp_ext_report_req req;
>> + int ret, npages = 0, resp_len;
>> + struct snp_report_resp *resp;
>> + struct snp_report_req *rreq;
>> + unsigned long fw_err = 0;
>> +
>> + if (!arg->req_data || !arg->resp_data)
>> + return -EINVAL;
>> +
>> + /* Copy the request payload from the userspace */
>> + if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
>> + return -EFAULT;
>> +
>> + rreq = &req.data;
>> +
>> + /* Message version must be non-zero */
>> + if (!rreq->msg_version)
>> + return -EINVAL;
>> +
>> + if (req.certs_len) {
>> + if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
>> + !IS_ALIGNED(req.certs_len, PAGE_SIZE))
>> + return -EINVAL;
>> + }
>> +
>> + if (req.certs_address && req.certs_len) {
>> + if (!access_ok(req.certs_address, req.certs_len))
>> + return -EFAULT;
>> +
>> + /*
>> + * Initialize the intermediate buffer with all zero's. This buffer
>> + * is used in the guest request message to get the certs blob from
>> + * the host. If host does not supply any certs in it, then we copy
>> + * zeros to indicate that certificate data was not provided.
>> + */
>> + memset(snp_dev->certs_data, 0, req.certs_len);
>> +
>> + input.data_gpa = __pa(snp_dev->certs_data);
>> + npages = req.certs_len >> PAGE_SHIFT;
>> + }
>> +
>> + /*
>> + * 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);
>> + if (!resp)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(resp, (void __user *)arg->resp_data, sizeof(*resp))) {
>> + ret = -EFAULT;
>> + goto e_free;
>> + }
>> +
>> + /* Encrypt the userspace provided payload */
>> + ret = enc_payload(snp_dev, rreq->msg_version, SNP_MSG_REPORT_REQ,
>> + &rreq->user_data, sizeof(rreq->user_data));
>> + if (ret)
>> + goto e_free;
>> +
>> + /* Call firmware to process the request */
>> + input.req_gpa = __pa(snp_dev->request);
>> + input.resp_gpa = __pa(snp_dev->response);
>> + input.data_npages = npages;
>> + memset(snp_dev->response, 0, sizeof(*snp_dev->response));
>> + ret = snp_issue_guest_request(EXT_GUEST_REQUEST, &input, &fw_err);
>> +
>> + /* Popogate any firmware error to the userspace */
>> + arg->fw_err = fw_err;
>> +
>> + /* If certs length is invalid then copy the returned length */
>> + if (arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) {
>> + req.certs_len = input.data_npages << PAGE_SHIFT;
>> +
>> + if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
>> + ret = -EFAULT;
>> +
>> + goto e_free;
>> + }
>> +
>> + if (ret)
>> + goto e_free;
>> +
>> + /* Decrypt the response payload */
>> + ret = verify_and_dec_payload(snp_dev, resp->data, resp_len);
>> + if (ret)
>> + goto e_free;
>> +
>> + /* Copy the certificate data blob to userspace */
>> + if (req.certs_address &&
>> + copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
>> + req.certs_len)) {
>> + ret = -EFAULT;
>> + goto e_free;
>> + }
>> +
>> + /* Copy the response payload to userspace */
>> + if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
>> + ret = -EFAULT;
>> +
>> +e_free:
>> + kfree(resp);
>> + return ret;
>> +}
>> +
>> static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>> {
>> struct snp_guest_dev *snp_dev = to_snp_dev(file);
>> @@ -368,6 +480,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
>> ret = get_derived_key(snp_dev, &input);
>> break;
>> }
>> + case SNP_GET_EXT_REPORT: {
>> + ret = get_ext_report(snp_dev, &input);
>> + break;
>> + }
>> default:
>> break;
>> }
>> @@ -453,6 +569,12 @@ static int __init snp_guest_probe(struct platform_device *pdev)
>> goto e_free_req;
>> }
>>
>> + snp_dev->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE);
>> + if (IS_ERR(snp_dev->certs_data)) {
>> + ret = PTR_ERR(snp_dev->certs_data);
>> + goto e_free_resp;
>> + }
>> +
>> misc = &snp_dev->misc;
>> misc->minor = MISC_DYNAMIC_MINOR;
>> misc->name = DEVICE_NAME;
>> @@ -460,6 +582,9 @@ static int __init snp_guest_probe(struct platform_device *pdev)
>>
>> return misc_register(misc);
>>
>> +e_free_resp:
>> + free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
>> +
>> e_free_req:
>> free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg));
>>
>> @@ -475,6 +600,7 @@ static int __exit snp_guest_remove(struct platform_device *pdev)
>>
>> free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg));
>> free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
>> + free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE);
>> deinit_crypto(snp_dev->crypto);
>> misc_deregister(&snp_dev->misc);
>>
>> diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h
>> index 621a9167df7a..23659215fcfb 100644
>> --- a/include/uapi/linux/sev-guest.h
>> +++ b/include/uapi/linux/sev-guest.h
>> @@ -57,6 +57,16 @@ struct snp_derived_key_resp {
>> __u8 data[64];
>> };
>>
>> +struct snp_ext_report_req {
>> + struct snp_report_req data;
>> +
>> + /* where to copy the certificate blob */
>> + __u64 certs_address;
>> +
>> + /* length of the certificate blob */
>> + __u32 certs_len;
>> +};
>> +
>> #define SNP_GUEST_REQ_IOC_TYPE 'S'
>>
>> /* Get SNP attestation report */
>> @@ -65,4 +75,7 @@ struct snp_derived_key_resp {
>> /* Get a derived key from the root */
>> #define SNP_GET_DERIVED_KEY _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x1, struct snp_user_guest_request)
>>
>> +/* Get SNP extended report as defined in the GHCB specification version 2. */
>> +#define SNP_GET_EXT_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x2, struct snp_user_guest_request)
>> +
>> #endif /* __UAPI_LINUX_SEV_GUEST_H_ */
>> --
>> 2.17.1
>>
>>