| Message ID | 20210820151933.22401-39-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
Hi Brijesh, On 20/08/2021 18:19, Brijesh Singh 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 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. It seems like the SNP_GET_EXT_REPORT ioctl does everything that the SNP_GET_REPORT ioctl does, and more. Why expose SNP_GET_REPORT to userspace at all? -Dov > 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 <brijesh.singh@amd.com> > --- > 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(+) > [...]
On 8/31/21 3:22 PM, Dov Murik wrote: > Hi Brijesh, > > On 20/08/2021 18:19, Brijesh Singh 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 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. > > It seems like the SNP_GET_EXT_REPORT ioctl does everything that the > SNP_GET_REPORT ioctl does, and more. Why expose SNP_GET_REPORT to > userspace at all? > > Since both of these options are provided by the GHCB protocol so I exposed it. Its possible that some applications may not care about the extended certificate blob. And in those case, if the hypervisor is programmed with the extended certificate blob and caller does not supply the enough number of pages to copy the blob then command should fail. This will enforce a new requirement on that guest application to allocate an extra memory. e.g: 1. Hypervisor is programmed with a system wide certificate blob using the SNP_SET_EXT_CONFIG ioctl(). 2. Guest wants to get the report but does not care about the certificate blob. 3. Guest issues a extended guest report with the npages = 0. The command will fail with invalid length and number of pages will be returned in the response. 4. Guest will not need to allocate memory to hold the certificate and reissue the command. The #4 is unnecessary for a guest which does not want to get. In this case, a guest can simply call the attestation report without asking for certificate blob. Please see the GHCB spec for more details. thanks
On 01/09/2021 0:11, Brijesh Singh wrote: > > > On 8/31/21 3:22 PM, Dov Murik wrote: >> Hi Brijesh, >> >> On 20/08/2021 18:19, Brijesh Singh 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 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. >> >> It seems like the SNP_GET_EXT_REPORT ioctl does everything that the >> SNP_GET_REPORT ioctl does, and more. Why expose SNP_GET_REPORT to >> userspace at all? >> >> > > Since both of these options are provided by the GHCB protocol so I > exposed it. Its possible that some applications may not care about the > extended certificate blob. And in those case, if the hypervisor is > programmed with the extended certificate blob and caller does not supply > the enough number of pages to copy the blob then command should fail. > This will enforce a new requirement on that guest application to > allocate an extra memory. e.g: > > 1. Hypervisor is programmed with a system wide certificate blob using > the SNP_SET_EXT_CONFIG ioctl(). > > 2. Guest wants to get the report but does not care about the certificate > blob. > > 3. Guest issues a extended guest report with the npages = 0. The command > will fail with invalid length and number of pages will be returned in > the response. > > 4. Guest will not need to allocate memory to hold the certificate and > reissue the command. > > The #4 is unnecessary for a guest which does not want to get. In this > case, a guest can simply call the attestation report without asking for > certificate blob. Please see the GHCB spec for more details. > OK. Originally I thought that by passing certs_address=NULL and certs_len=0 the user program can say "I don't want this extra data"; but now I understand that this will return an error (invalid length) with number of pages needed. -Dov
On Fri, Aug 20, 2021 at 10:19:33AM -0500, Brijesh Singh wrote: > Version 2 of GHCB specification defines NAE to get the extended guest Resolve "NAE" pls. > request. It is similar to the SNP_GET_REPORT ioctl. The main difference > is related to the additional data that be returned. The additional "that will be returned" > 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 <brijesh.singh@amd.com> > --- > 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 "will contain" > +and snp_ext_report_req.certs_address will contains the certificate blob. If the ditto. > +length of the blob is lesser than expected then snp_ext_report_req.certs_len will "is smaller" > +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 */ "from 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 Please use passive voice: no "we" or "I", etc, > + * 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; This one is really confusing. You assign ret in the if branch above but then you test ret outside too, just in case the snp_issue_guest_request() call above has failed. But then if that call has failed, you still go and do some cleanup work for invalid certs length... So that get_ext_report() function is doing too many things at once and is crying to be split. For example, the glue around snp_issue_guest_request() is already carved out in handle_guest_request(). Why aren't you calling that function here too? That'll keep the enc, request, dec payload game separate and then the rest of the logic can remain in get_ext_report()... ... > 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; > + } Same comments here as for patch 37. > + > misc = &snp_dev->misc; > misc->minor = MISC_DYNAMIC_MINOR; > misc->name = DEVICE_NAME;
* Brijesh Singh (brijesh.singh@amd.com) 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 <brijesh.singh@amd.com> 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? 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 > >
Hi Boris, On 9/8/21 12:53 PM, Borislav Petkov wrote: >> + >> + /* 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; > This one is really confusing. You assign ret in the if branch > above but then you test ret outside too, just in case the > snp_issue_guest_request() call above has failed. > > But then if that call has failed, you still go and do some cleanup work > for invalid certs length... > > So that get_ext_report() function is doing too many things at once and > is crying to be split. I will try to see what I can come up with to make it easy to read. > > For example, the glue around snp_issue_guest_request() is already carved > out in handle_guest_request(). Why aren't you calling that function here > too? The handle_guest_request() uses the VMGEXIT_GUEST_REQUEST which does not require the memory for the certificate blobs etc. But based on your earlier comment that we should let the driver use the VMGEXIT code rather than enum will help in this case. I will be reworking handle_guest_request() so that it can be used for both the cases (with and without certificate). > That'll keep the enc, request, dec payload game separate and then the > rest of the logic can remain in get_ext_report()... > > ... > >> 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; >> + } > Same comments here as for patch 37. > >> + >> misc = &snp_dev->misc; >> misc->minor = MISC_DYNAMIC_MINOR; >> misc->name = DEVICE_NAME; >
On 9/15/21 5:02 AM, Dr. David Alan Gilbert wrote: > * Brijesh Singh (brijesh.singh@amd.com) 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 <brijesh.singh@amd.com> > 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 >> >>
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_ */
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 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 <brijesh.singh@amd.com> --- 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(+)