| Message ID | 20220128171804.569796-43-brijesh.singh@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Fri, Jan 28, 2022 at 10:19 AM Brijesh Singh <brijesh.singh@amd.com> wrote: > > The SNP_GET_DERIVED_KEY ioctl interface can be used by the SNP guest to > ask the firmware to provide a key derived from a root key. The derived > key may be used by the guest for any purposes it chooses, such as a > sealing key or communicating with the external entities. > > See SEV-SNP firmware spec for more information. > > Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> Reviewed-by: Peter Gonda <pgonda@google.com> > --- > Documentation/virt/coco/sevguest.rst | 17 ++++++++++ > drivers/virt/coco/sevguest/sevguest.c | 45 +++++++++++++++++++++++++++ > include/uapi/linux/sev-guest.h | 17 ++++++++++ > 3 files changed, 79 insertions(+) > > diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst > index 47ef3b0821d5..aafc9bce9aef 100644 > --- a/Documentation/virt/coco/sevguest.rst > +++ b/Documentation/virt/coco/sevguest.rst > @@ -72,6 +72,23 @@ On success, the snp_report_resp.data will contains the report. The report > contain the format described in the SEV-SNP specification. See the SEV-SNP > specification for further details. > > +2.2 SNP_GET_DERIVED_KEY > +----------------------- > +:Technology: sev-snp > +:Type: guest ioctl > +:Parameters (in): struct snp_derived_key_req > +:Returns (out): struct snp_derived_key_resp on success, -negative on error > + > +The SNP_GET_DERIVED_KEY ioctl can be used to get a key derive from a root key. derived from ... > +The derived key can be used by the guest for any purpose, such as sealing keys > +or communicating with external entities. Question: How would this be used to communicate with external entities? Reading Section 7.2 it seems like we could pick the VCEK and have no guest specific inputs and we'd get the same derived key as we would on another guest on the same platform with, is that correct? > + > +The ioctl uses the SNP_GUEST_REQUEST (MSG_KEY_REQ) command provided by the > +SEV-SNP firmware to derive the key. See SEV-SNP specification for further details > +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. > > Reference > --------- > diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c > index 6dc0785ddd4b..4369e55df9a6 100644 > --- a/drivers/virt/coco/sevguest/sevguest.c > +++ b/drivers/virt/coco/sevguest/sevguest.c > @@ -392,6 +392,48 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io > return rc; > } > > +static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) > +{ > + struct snp_guest_crypto *crypto = snp_dev->crypto; > + struct snp_derived_key_resp resp = {0}; > + struct snp_derived_key_req req = {0}; > + int rc, resp_len; > + u8 buf[64+16]; /* Response data is 64 bytes and max authsize for GCM is 16 bytes */ > + > + 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; > + > + /* > + * 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, arg->msg_version, > + SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len, > + &arg->fw_err); > + if (rc) > + goto e_free; > + > + /* Copy the response payload to userspace */ > + memcpy(resp.data, buf, sizeof(resp.data)); > + if (copy_to_user((void __user *)arg->resp_data, &resp, sizeof(resp))) > + rc = -EFAULT; > + > +e_free: > + memzero_explicit(buf, sizeof(buf)); > + memzero_explicit(&resp, sizeof(resp)); > + return rc; > +} > + > static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) > { > struct snp_guest_dev *snp_dev = to_snp_dev(file); > @@ -421,6 +463,9 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long > case SNP_GET_REPORT: > ret = get_report(snp_dev, &input); > break; > + case SNP_GET_DERIVED_KEY: > + ret = get_derived_key(snp_dev, &input); > + break; > default: > break; > } > diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h > index 081d314a6279..bcd00a6d4501 100644 > --- a/include/uapi/linux/sev-guest.h > +++ b/include/uapi/linux/sev-guest.h > @@ -30,6 +30,20 @@ struct snp_report_resp { > __u8 data[4000]; > }; > > +struct snp_derived_key_req { > + __u32 root_key_select; > + __u32 rsvd; > + __u64 guest_field_select; > + __u32 vmpl; > + __u32 guest_svn; > + __u64 tcb_version; > +}; > + > +struct snp_derived_key_resp { > + /* response data, see SEV-SNP spec for the format */ > + __u8 data[64]; > +}; > + > struct snp_guest_request_ioctl { > /* message version number (must be non-zero) */ > __u8 msg_version; > @@ -47,4 +61,7 @@ struct snp_guest_request_ioctl { > /* Get SNP attestation report */ > #define SNP_GET_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x0, 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) > + > #endif /* __UAPI_LINUX_SEV_GUEST_H_ */ > -- > 2.25.1 >
On 2/1/22 2:39 PM, Peter Gonda wrote: > On Fri, Jan 28, 2022 at 10:19 AM Brijesh Singh <brijesh.singh@amd.com> wrote: >> The SNP_GET_DERIVED_KEY ioctl interface can be used by the SNP guest to >> ask the firmware to provide a key derived from a root key. The derived >> key may be used by the guest for any purposes it chooses, such as a >> sealing key or communicating with the external entities. >> >> See SEV-SNP firmware spec for more information. >> >> Reviewed-by: Liam Merwick <liam.merwick@oracle.com> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > Reviewed-by: Peter Gonda <pgonda@google.com> > >> --- >> Documentation/virt/coco/sevguest.rst | 17 ++++++++++ >> drivers/virt/coco/sevguest/sevguest.c | 45 +++++++++++++++++++++++++++ >> include/uapi/linux/sev-guest.h | 17 ++++++++++ >> 3 files changed, 79 insertions(+) >> >> diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst >> index 47ef3b0821d5..aafc9bce9aef 100644 >> --- a/Documentation/virt/coco/sevguest.rst >> +++ b/Documentation/virt/coco/sevguest.rst >> @@ -72,6 +72,23 @@ On success, the snp_report_resp.data will contains the report. The report >> contain the format described in the SEV-SNP specification. See the SEV-SNP >> specification for further details. >> >> +2.2 SNP_GET_DERIVED_KEY >> +----------------------- >> +:Technology: sev-snp >> +:Type: guest ioctl >> +:Parameters (in): struct snp_derived_key_req >> +:Returns (out): struct snp_derived_key_resp on success, -negative on error >> + >> +The SNP_GET_DERIVED_KEY ioctl can be used to get a key derive from a root key. > derived from ... > >> +The derived key can be used by the guest for any purpose, such as sealing keys >> +or communicating with external entities. > Question: How would this be used to communicate with external > entities? Reading Section 7.2 it seems like we could pick the VCEK and > have no guest specific inputs and we'd get the same derived key as we > would on another guest on the same platform with, is that correct? That could work. This method is using the idea that the guests would derive identical keys removing the need for a complex key establishment protocol. However, it's probably better to approach this slightly differently. The derived key can be used to produce a persistent guest identity key that can be recovered across instances. That key can sign an key establishment exchange (e.g. an ephemeral ECDH key) for establishing trusted channels with remote parties. Further, that identity key could be signed by a centralized CA. A way to approach that could be placing the fingerprint of the identity key into the REPORT_DATA parameter of the attestation request message. The attestation report will come back signed by the security processor and will contain the fingerprint. A CSR to the CA can be accompanied by the attestation report to prove the key came from the guest described in the attestation report. If the guest does not require keys or secrets to be persisted, or if the guest has other means for persisting keys or secrets, the derivation messages are not necessary. It's an optional security primitive for guests to use. thanks
On Fri, Jan 28, 2022 at 11:18:03AM -0600, Brijesh Singh wrote: > +static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) > +{ > + struct snp_guest_crypto *crypto = snp_dev->crypto; > + struct snp_derived_key_resp resp = {0}; > + struct snp_derived_key_req req = {0}; > + int rc, resp_len; > + u8 buf[64+16]; /* Response data is 64 bytes and max authsize for GCM is 16 bytes */ verify_comment_style: Warning: No tail comments please: drivers/virt/coco/sevguest/sevguest.c:401 [+ u8 buf[64+16]; /* Response data is 64 bytes and max authsize for GCM is 16 bytes */] > + if (!arg->req_data || !arg->resp_data) > + return -EINVAL; > + > + /* Copy the request payload from userspace */ That comment looks useless. > + if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req))) > + return -EFAULT; > + > + /* > + * 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; That test can happen before the copy_from_user() above. > + > + /* Issue the command to get the attestation report */ Also useless. > + rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg->msg_version, > + SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len, > + &arg->fw_err); > + if (rc) > + goto e_free; > + > + /* Copy the response payload to userspace */ Ditto. > + memcpy(resp.data, buf, sizeof(resp.data)); > + if (copy_to_user((void __user *)arg->resp_data, &resp, sizeof(resp))) > + rc = -EFAULT; > + > +e_free: > + memzero_explicit(buf, sizeof(buf)); > + memzero_explicit(&resp, sizeof(resp)); Those are allocated on stack, why are you clearing them? > + return rc; > +}
On 2/7/22 2:52 AM, Borislav Petkov wrote:
> Those are allocated on stack, why are you clearing them?
Yep, no need to explicitly clear it. I'll take it out in next rev.
thanks
On 07/02/2022 18:23, Brijesh Singh wrote: > > > On 2/7/22 2:52 AM, Borislav Petkov wrote: >> Those are allocated on stack, why are you clearing them? > > Yep, no need to explicitly clear it. I'll take it out in next rev. > Wait, this is key material generated by PSP and passed to userspace. Why leave copies of it floating around kernel memory? I thought that's the whole reason for these memzero_explicit() calls (maybe add a comment?). As an example, in arch/x86/crypto/aesni-intel_glue.c there are two calls to memzero_explicit(), both on stack variables; the only reason for these calls (as I understand it) is to avoid some future possible leak of this sensitive data (keys, cipher context, etc.). I'm sure there are other examples in the kernel code. -Dov
On 2/7/22 1:09 PM, Dov Murik wrote: > > > On 07/02/2022 18:23, Brijesh Singh wrote: >> >> >> On 2/7/22 2:52 AM, Borislav Petkov wrote: >>> Those are allocated on stack, why are you clearing them? >> >> Yep, no need to explicitly clear it. I'll take it out in next rev. >> > > Wait, this is key material generated by PSP and passed to userspace. > Why leave copies of it floating around kernel memory? I thought that's > the whole reason for these memzero_explicit() calls (maybe add a comment?). > Ah, now I remember I added the memzero_explicit() to address your review feedback :) In that patch version, we were using the kmalloc() to store the response data; since then, we switched to stack. We will leak the key outside when the stack is converted private-> shared; I don't know if any of these are going to happen. I can add a comment and keep the memzero_explicit() call. Boris, let me know if you are okay with it? > As an example, in arch/x86/crypto/aesni-intel_glue.c there are two calls > to memzero_explicit(), both on stack variables; the only reason for > these calls (as I understand it) is to avoid some future possible leak > of this sensitive data (keys, cipher context, etc.). I'm sure there are > other examples in the kernel code. >
On Mon, Feb 07, 2022 at 02:08:17PM -0600, Brijesh Singh wrote:
> Boris, let me know if you are okay with it?
Yes, most definitely as long as there's a comment explaining why.
Thx.
On 07/02/2022 22:08, Brijesh Singh wrote: > > > On 2/7/22 1:09 PM, Dov Murik wrote: >> >> >> On 07/02/2022 18:23, Brijesh Singh wrote: >>> >>> >>> On 2/7/22 2:52 AM, Borislav Petkov wrote: >>>> Those are allocated on stack, why are you clearing them? >>> >>> Yep, no need to explicitly clear it. I'll take it out in next rev. >>> >> >> Wait, this is key material generated by PSP and passed to userspace. >> Why leave copies of it floating around kernel memory? I thought that's >> the whole reason for these memzero_explicit() calls (maybe add a >> comment?). >> > > > Ah, now I remember I added the memzero_explicit() to address your review > feedback :) In that patch version, we were using the kmalloc() to store > the response data; since then, we switched to stack. We will leak the > key outside when the stack is converted private-> shared; I don't know > if any of these are going to happen. I can add a comment and keep the > memzero_explicit() call. > Just to be clear, I didn't mean necessarily "leak the key to the untrusted host" (even if a page is converted back from private to shared, it is encrypted, so host can't read its contents). But even *inside* the guest, when dealing with sensitive data like keys, we should minimize the amount of copies that float around (I assume this is the reason for most of the uses of memzero_explicit() in the kernel). -Dov > Boris, let me know if you are okay with it? > > >> As an example, in arch/x86/crypto/aesni-intel_glue.c there are two calls >> to memzero_explicit(), both on stack variables; the only reason for >> these calls (as I understand it) is to avoid some future possible leak >> of this sensitive data (keys, cipher context, etc.). I'm sure there are >> other examples in the kernel code. >>
On Tue, Feb 08, 2022 at 09:56:52AM +0200, Dov Murik wrote: > Just to be clear, I didn't mean necessarily "leak the key to the > untrusted host" (even if a page is converted back from private to > shared, it is encrypted, so host can't read its contents). But even > *inside* the guest, when dealing with sensitive data like keys, we > should minimize the amount of copies that float around (I assume this is > the reason for most of the uses of memzero_explicit() in the kernel). I don't know about Brijesh but I understood you exactly as you mean it. And yap, I agree we should always clear such sensitive buffers.
On 2/8/22 1:56 AM, Dov Murik wrote: ... > > Just to be clear, I didn't mean necessarily "leak the key to the > untrusted host" (even if a page is converted back from private to > shared, it is encrypted, so host can't read its contents). But even > *inside* the guest, when dealing with sensitive data like keys, we > should minimize the amount of copies that float around (I assume this is > the reason for most of the uses of memzero_explicit() in the kernel). > Yap, I agree with your point and will keep the memzero_explicit(). -Brijesh
diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst index 47ef3b0821d5..aafc9bce9aef 100644 --- a/Documentation/virt/coco/sevguest.rst +++ b/Documentation/virt/coco/sevguest.rst @@ -72,6 +72,23 @@ On success, the snp_report_resp.data will contains the report. The report contain the format described in the SEV-SNP specification. See the SEV-SNP specification for further details. +2.2 SNP_GET_DERIVED_KEY +----------------------- +:Technology: sev-snp +:Type: guest ioctl +:Parameters (in): struct snp_derived_key_req +:Returns (out): struct snp_derived_key_resp on success, -negative on error + +The SNP_GET_DERIVED_KEY ioctl can be used to get a key derive from a root key. +The derived key can be used by the guest for any purpose, such as sealing keys +or communicating with external entities. + +The ioctl uses the SNP_GUEST_REQUEST (MSG_KEY_REQ) command provided by the +SEV-SNP firmware to derive the key. See SEV-SNP specification for further details +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. Reference --------- diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c index 6dc0785ddd4b..4369e55df9a6 100644 --- a/drivers/virt/coco/sevguest/sevguest.c +++ b/drivers/virt/coco/sevguest/sevguest.c @@ -392,6 +392,48 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io return rc; } +static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) +{ + struct snp_guest_crypto *crypto = snp_dev->crypto; + struct snp_derived_key_resp resp = {0}; + struct snp_derived_key_req req = {0}; + int rc, resp_len; + u8 buf[64+16]; /* Response data is 64 bytes and max authsize for GCM is 16 bytes */ + + 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; + + /* + * 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, arg->msg_version, + SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len, + &arg->fw_err); + if (rc) + goto e_free; + + /* Copy the response payload to userspace */ + memcpy(resp.data, buf, sizeof(resp.data)); + if (copy_to_user((void __user *)arg->resp_data, &resp, sizeof(resp))) + rc = -EFAULT; + +e_free: + memzero_explicit(buf, sizeof(buf)); + memzero_explicit(&resp, sizeof(resp)); + return rc; +} + static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) { struct snp_guest_dev *snp_dev = to_snp_dev(file); @@ -421,6 +463,9 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long case SNP_GET_REPORT: ret = get_report(snp_dev, &input); break; + case SNP_GET_DERIVED_KEY: + ret = get_derived_key(snp_dev, &input); + break; default: break; } diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h index 081d314a6279..bcd00a6d4501 100644 --- a/include/uapi/linux/sev-guest.h +++ b/include/uapi/linux/sev-guest.h @@ -30,6 +30,20 @@ struct snp_report_resp { __u8 data[4000]; }; +struct snp_derived_key_req { + __u32 root_key_select; + __u32 rsvd; + __u64 guest_field_select; + __u32 vmpl; + __u32 guest_svn; + __u64 tcb_version; +}; + +struct snp_derived_key_resp { + /* response data, see SEV-SNP spec for the format */ + __u8 data[64]; +}; + struct snp_guest_request_ioctl { /* message version number (must be non-zero) */ __u8 msg_version; @@ -47,4 +61,7 @@ struct snp_guest_request_ioctl { /* Get SNP attestation report */ #define SNP_GET_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x0, 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) + #endif /* __UAPI_LINUX_SEV_GUEST_H_ */