| Message ID | 20220224165625.2175020-45-brijesh.singh@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Thu, Feb 24, 2022 at 10:56:24AM -0600, Brijesh Singh wrote: > +static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) > +{ > + struct snp_guest_crypto *crypto = snp_dev->crypto; > + struct snp_ext_report_req req = {0}; > + struct snp_report_resp *resp; > + int ret, npages = 0, resp_len; > + > + lockdep_assert_held(&snp_cmd_mutex); > + > + if (!arg->req_data || !arg->resp_data) > + return -EINVAL; > + > + if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req))) > + return -EFAULT; > + > + 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 zeros. 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 copy > + * zeros to indicate that certificate data was not provided. > + */ > + memset(snp_dev->certs_data, 0, req.certs_len); > + > + npages = req.certs_len >> PAGE_SHIFT; > + } I think all those checks should be made more explicit. This makes the code a lot more readable and straight-forward (pasting the full excerpt because the incremental diff ontop is less readable): ... if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req))) return -EFAULT; if (!req.certs_len || !req.certs_address) return -EINVAL; if (req.certs_len > SEV_FW_BLOB_MAX_SIZE || !IS_ALIGNED(req.certs_len, PAGE_SIZE)) return -EINVAL; if (!access_ok(req.certs_address, req.certs_len)) return -EFAULT; /* * Initialize the intermediate buffer with all zeros. 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 copy * zeros to indicate that certificate data was not provided. */ memset(snp_dev->certs_data, 0, req.certs_len); npages = req.certs_len >> PAGE_SHIFT;
Hi Boris, On 3/3/22 09:28, Borislav Petkov wrote: > On Thu, Feb 24, 2022 at 10:56:24AM -0600, Brijesh Singh wrote: >> +static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) >> +{ >> + struct snp_guest_crypto *crypto = snp_dev->crypto; >> + struct snp_ext_report_req req = {0}; >> + struct snp_report_resp *resp; >> + int ret, npages = 0, resp_len; >> + >> + lockdep_assert_held(&snp_cmd_mutex); >> + >> + if (!arg->req_data || !arg->resp_data) >> + return -EINVAL; >> + >> + if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req))) >> + return -EFAULT; >> + >> + 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 zeros. 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 copy >> + * zeros to indicate that certificate data was not provided. >> + */ >> + memset(snp_dev->certs_data, 0, req.certs_len); >> + >> + npages = req.certs_len >> PAGE_SHIFT; >> + } > > I think all those checks should be made more explicit. This makes the > code a lot more readable and straight-forward (pasting the full excerpt > because the incremental diff ontop is less readable): > > ... > > if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req))) > return -EFAULT; > > if (!req.certs_len || !req.certs_address) > return -EINVAL; > > if (req.certs_len > SEV_FW_BLOB_MAX_SIZE || > !IS_ALIGNED(req.certs_len, PAGE_SIZE)) > return -EINVAL; > I did not fail on !req.cert_len, because my read of the GHCB spec says that additional data (certificate blob) is optional. A user could call SNP_GET_EXT_REPORT without asking for the extended certificate. In this case, SNP_GET_EXT_REPORT == SNP_GET_REPORT. Text from the GHCB spec section 4.1.8 --------------- https://developer.amd.com/wp-content/resources/56421.pdf The SNP Extended Guest Request NAE event is very similar to the SNP Guest Request NAE event. The difference is related to the additional data that can be returned based on the guest request. Any SNP Guest Request that does not support returning additional data must execute as if invoked as an SNP Guest Request. -------------- > if (!access_ok(req.certs_address, req.certs_len)) > return -EFAULT; > > /* > * Initialize the intermediate buffer with all zeros. 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 copy > * zeros to indicate that certificate data was not provided. > */ > memset(snp_dev->certs_data, 0, req.certs_len); > > npages = req.certs_len >> PAGE_SHIFT; >
On Thu, Mar 03, 2022 at 10:47:20AM -0600, Brijesh Singh wrote: > I did not fail on !req.cert_len, because my read of the GHCB spec says that > additional data (certificate blob) is optional. A user could call > SNP_GET_EXT_REPORT without asking for the extended certificate. In this > case, SNP_GET_EXT_REPORT == SNP_GET_REPORT. > > Text from the GHCB spec section 4.1.8 > --------------- > https://developer.amd.com/wp-content/resources/56421.pdf > > The SNP Extended Guest Request NAE event is very similar to the SNP Guest > Request NAE event. The difference is related to the additional data that can > be returned based on the guest request. Any SNP Guest Request that does not > support returning additional data must execute as if invoked as an SNP Guest > Request. > -------------- Sorry, it is still not clear to me how "without asking for the extended certificate" == !req.certs_len That's not explained in the help text either. And ->certs_len is part of the input structure but nowhere does it say that when that thing is 0, the request will be downgraded to a SNP_GET_REPORT. How is the user of this supposed to know? And regardless, you can still streamline the code as in the example I gave so that it is clear which values are checked and for which does the request get failed...
On 3/4/22 8:06 AM, Borislav Petkov wrote: > On Thu, Mar 03, 2022 at 10:47:20AM -0600, Brijesh Singh wrote: >> I did not fail on !req.cert_len, because my read of the GHCB spec says that >> additional data (certificate blob) is optional. A user could call >> SNP_GET_EXT_REPORT without asking for the extended certificate. In this >> case, SNP_GET_EXT_REPORT == SNP_GET_REPORT. >> >> Text from the GHCB spec section 4.1.8 >> --------------- >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdeveloper.amd.com%2Fwp-content%2Fresources%2F56421.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7C10f73f9bd6524f9f9c4e08d9fde843d5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637819996563585169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=GS3iyUCAOmaPuxN4sRi%2B%2FFTsRGF7QC1jISc%2B25c4XtY%3D&reserved=0 >> >> The SNP Extended Guest Request NAE event is very similar to the SNP Guest >> Request NAE event. The difference is related to the additional data that can >> be returned based on the guest request. Any SNP Guest Request that does not >> support returning additional data must execute as if invoked as an SNP Guest >> Request. >> -------------- > Sorry, it is still not clear to me how > > "without asking for the extended certificate" == !req.certs_len > > That's not explained in the help text either. And ->certs_len is part of > the input structure but nowhere does it say that when that thing is 0, > the request will be downgraded to a SNP_GET_REPORT. The decision to downgrade will be done by the hypervisor. See the GHCB spec (page 36), if RBX (aka number of certificate data pages) is zero then section SNP_GET_EXT_REQUEST section (4.1.8) say that hypervisor must treat it as SNP_GET_REQUEST. > > How is the user of this supposed to know? Depending on which ioctl user want to use for querying the attestation report, she need to look at the SNP/GHCB specification for more details. The blob contains header that application need to parse to get to the actual certificate. The header is defined in the spec. From kernel driver point-of-view, all these are opaque data. > > And regardless, you can still streamline the code as in the example I > gave so that it is clear which values are checked and for which does the > request get failed... Will do. thanks
On Fri, Mar 04, 2022 at 09:39:16AM -0600, Brijesh Singh wrote: > Depending on which ioctl user want to use for querying the attestation > report, she need to look at the SNP/GHCB specification for more details. > The blob contains header that application need to parse to get to the > actual certificate. The header is defined in the spec. From kernel > driver point-of-view, all these are opaque data. ... and the ioctl text needs to point to the spec so that the user knows where to find everything needed. Or how do you expect people to know how to use those ioctls? > Will do. Thx!
On 3/4/22 9:53 AM, Borislav Petkov wrote: > On Fri, Mar 04, 2022 at 09:39:16AM -0600, Brijesh Singh wrote: >> Depending on which ioctl user want to use for querying the attestation >> report, she need to look at the SNP/GHCB specification for more details. >> The blob contains header that application need to parse to get to the >> actual certificate. The header is defined in the spec. From kernel >> driver point-of-view, all these are opaque data. > ... and the ioctl text needs to point to the spec so that the user knows > where to find everything needed. Or how do you expect people to know how > to use those ioctls? I did added a text in Documentation/virt/coco/sevguest.rst (section 2.3) that user need to look the GHCB spec for further detail.
On Fri, Mar 04, 2022 at 10:03:52AM -0600, Brijesh Singh wrote: > I did added a text in Documentation/virt/coco/sevguest.rst (section 2.3) > that user need to look the GHCB spec for further detail. If you mean this: "See GHCB specification for further detail on how to parse the certificate blob." that ain't even scratching the surface. You need to explain that magic "certs_len == 0" condition in that textm how it is handled and what that causes the HV to do. You need to realize that because it is clear to you, it ain't that clear to others.
diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst index ae2e76f59435..0f352056572d 100644 --- a/Documentation/virt/coco/sevguest.rst +++ b/Documentation/virt/coco/sevguest.rst @@ -95,6 +95,29 @@ on the various fields passed in the key derivation request. On success, the snp_derived_key_resp.data contains the derived key value. See the SEV-SNP specification for further details. + +2.3 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 contain the attestation report +and snp_ext_report_req.certs_address will contain the certificate blob. If the +length of the blob is smaller 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. + Reference --------- diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c index 2062f94fdc38..b840cf426f38 100644 --- a/drivers/virt/coco/sevguest/sevguest.c +++ b/drivers/virt/coco/sevguest/sevguest.c @@ -41,6 +41,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; struct snp_secrets_page_layout *layout; @@ -431,6 +432,83 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque return rc; } +static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) +{ + struct snp_guest_crypto *crypto = snp_dev->crypto; + struct snp_ext_report_req req = {0}; + struct snp_report_resp *resp; + int ret, npages = 0, resp_len; + + lockdep_assert_held(&snp_cmd_mutex); + + if (!arg->req_data || !arg->resp_data) + return -EINVAL; + + if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req))) + return -EFAULT; + + 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 zeros. 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 copy + * zeros to indicate that certificate data was not provided. + */ + memset(snp_dev->certs_data, 0, req.certs_len); + + 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; + + snp_dev->input.data_npages = npages; + ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg->msg_version, + SNP_MSG_REPORT_REQ, &req.data, + sizeof(req.data), resp->data, resp_len, &arg->fw_err); + + /* If certs length is invalid then copy the returned length */ + if (arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) { + req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT; + + if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req))) + ret = -EFAULT; + } + + if (ret) + goto e_free; + + if (req.certs_address && req.certs_len && + copy_to_user((void __user *)req.certs_address, snp_dev->certs_data, + req.certs_len)) { + ret = -EFAULT; + goto e_free; + } + + 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); @@ -463,6 +541,9 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long case SNP_GET_DERIVED_KEY: ret = get_derived_key(snp_dev, &input); break; + case SNP_GET_EXT_REPORT: + ret = get_ext_report(snp_dev, &input); + break; default: break; } @@ -590,10 +671,14 @@ static int __init snp_guest_probe(struct platform_device *pdev) if (!snp_dev->response) goto e_free_request; + snp_dev->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE); + if (!snp_dev->certs_data) + goto e_free_response; + ret = -EIO; snp_dev->crypto = init_crypto(snp_dev, snp_dev->vmpck, VMPCK_KEY_LEN); if (!snp_dev->crypto) - goto e_free_response; + goto e_free_cert_data; misc = &snp_dev->misc; misc->minor = MISC_DYNAMIC_MINOR; @@ -603,14 +688,17 @@ static int __init snp_guest_probe(struct platform_device *pdev) /* initial the input address for guest request */ snp_dev->input.req_gpa = __pa(snp_dev->request); snp_dev->input.resp_gpa = __pa(snp_dev->response); + snp_dev->input.data_gpa = __pa(snp_dev->certs_data); ret = misc_register(misc); if (ret) - goto e_free_response; + goto e_free_cert_data; dev_info(dev, "Initialized SNP guest driver (using vmpck_id %d)\n", vmpck_id); return 0; +e_free_cert_data: + free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE); e_free_response: free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg)); e_free_request: @@ -624,6 +712,7 @@ static int __exit snp_guest_remove(struct platform_device *pdev) { struct snp_guest_dev *snp_dev = platform_get_drvdata(pdev); + free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE); free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg)); free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg)); deinit_crypto(snp_dev->crypto); diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h index 598367f12064..256aaeff7e65 100644 --- a/include/uapi/linux/sev-guest.h +++ b/include/uapi/linux/sev-guest.h @@ -56,6 +56,16 @@ struct snp_guest_request_ioctl { __u64 fw_err; }; +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 */ @@ -64,4 +74,7 @@ struct snp_guest_request_ioctl { /* Get a derived key from the root */ #define SNP_GET_DERIVED_KEY _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x1, struct snp_guest_request_ioctl) +/* 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_guest_request_ioctl) + #endif /* __UAPI_LINUX_SEV_GUEST_H_ */
Version 2 of GHCB specification defines Non-Automatic-Exit(NAE) to get the extended guest report. It is similar to the SNP_GET_REPORT ioctl. The main difference is related to the additional data that will 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 | 23 +++++++ drivers/virt/coco/sevguest/sevguest.c | 93 ++++++++++++++++++++++++++- include/uapi/linux/sev-guest.h | 13 ++++ 3 files changed, 127 insertions(+), 2 deletions(-)