| Message ID | 20210820151933.22401-38-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
Hi Brijesh, On 20/08/2021 18:19, Brijesh Singh 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 choose, such as a > sealing key or communicating with the external entities. > > See SEV-SNP firmware spec for more information. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > Documentation/virt/coco/sevguest.rst | 18 ++++++++++ > drivers/virt/coco/sevguest/sevguest.c | 48 +++++++++++++++++++++++++++ > include/uapi/linux/sev-guest.h | 24 ++++++++++++++ > 3 files changed, 90 insertions(+) > > diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst > index 52d5915037ef..25446670d816 100644 > --- a/Documentation/virt/coco/sevguest.rst > +++ b/Documentation/virt/coco/sevguest.rst > @@ -67,3 +67,21 @@ provided by the SEV-SNP firmware to query the attestation report. > On success, the snp_report_resp.data will contains the report. The report > format is 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_req 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 fileds passed in the key derivation request. > + > +On success, the snp_derived_key_resp.data will contains the derived key > +value. > diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c > index d029a98ad088..621b1c5a9cfc 100644 > --- a/drivers/virt/coco/sevguest/sevguest.c > +++ b/drivers/virt/coco/sevguest/sevguest.c > @@ -303,6 +303,50 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_user_guest_reque > return rc; > } > > +static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *arg) > +{ > + struct snp_guest_crypto *crypto = snp_dev->crypto; > + struct snp_derived_key_resp *resp; > + struct snp_derived_key_req req; > + int rc, resp_len; > + > + 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; > + > + /* 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; > + resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); The length of resp->data is 64 bytes; I assume crypto->a_len is not a lot more (and probably known in advance for AES GCM). Maybe use a buffer on the stack instead of allocating and freeing? > + if (!resp) > + return -ENOMEM; > + > + /* Issue the command to get the attestation report */ > + rc = handle_guest_request(snp_dev, req.msg_version, SNP_MSG_KEY_REQ, > + &req.data, sizeof(req.data), resp->data, resp_len, > + &arg->fw_err); > + if (rc) > + goto e_free; > + > + /* Copy the response payload to userspace */ > + if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp))) > + rc = -EFAULT; > + > +e_free: > + kfree(resp); Since resp contains key material, I think you should explicit_memzero() it before freeing, so the key bytes don't linger around in unused memory. I'm not sure if any copies are made inside the handle_guest_request call above; maybe zero these as well. -Dov > + 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); > @@ -320,6 +364,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long > 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 e8cfd15133f3..621a9167df7a 100644 > --- a/include/uapi/linux/sev-guest.h > +++ b/include/uapi/linux/sev-guest.h > @@ -36,9 +36,33 @@ struct snp_user_guest_request { > __u64 fw_err; > }; > > +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_req { > + /* message version number (must be non-zero) */ > + __u8 msg_version; > + > + struct __snp_derived_key_req data; > +}; > + > +struct snp_derived_key_resp { > + /* response data, see SEV-SNP spec for the format */ > + __u8 data[64]; > +}; > + > #define SNP_GUEST_REQ_IOC_TYPE 'S' > > /* Get SNP attestation report */ > #define SNP_GET_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x0, struct snp_user_guest_request) > > +/* Get a derived key from the root */ > +#define SNP_GET_DERIVED_KEY _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x1, struct snp_user_guest_request) > + > #endif /* __UAPI_LINUX_SEV_GUEST_H_ */ >
Hi Dov, On 8/31/21 1:59 PM, Dov Murik wrote: >> + >> + /* >> + * 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); > > The length of resp->data is 64 bytes; I assume crypto->a_len is not a > lot more (and probably known in advance for AES GCM). Maybe use a > buffer on the stack instead of allocating and freeing? > The authtag size can be up to 16 bytes, so I guess I can allocate 80 bytes on stack and avoid the kzalloc(). > >> + if (!resp) >> + return -ENOMEM; >> + >> + /* Issue the command to get the attestation report */ >> + rc = handle_guest_request(snp_dev, req.msg_version, SNP_MSG_KEY_REQ, >> + &req.data, sizeof(req.data), resp->data, resp_len, >> + &arg->fw_err); >> + if (rc) >> + goto e_free; >> + >> + /* Copy the response payload to userspace */ >> + if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp))) >> + rc = -EFAULT; >> + >> +e_free: >> + kfree(resp); > > Since resp contains key material, I think you should explicit_memzero() > it before freeing, so the key bytes don't linger around in unused > memory. I'm not sure if any copies are made inside the > handle_guest_request call above; maybe zero these as well. > I can do that, but I guess I am trying to find a reason for it. The resp buffer is encrypted page, so, the key is protected from the hypervisor access. Are you thinking about an attack within the VM guest OS ? -Brijesh
On 01/09/2021 0:04, Brijesh Singh wrote: > Hi Dov, > > > On 8/31/21 1:59 PM, Dov Murik wrote: >>> + >>> + /* >>> + * 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); >> >> The length of resp->data is 64 bytes; I assume crypto->a_len is not a >> lot more (and probably known in advance for AES GCM). Maybe use a >> buffer on the stack instead of allocating and freeing? >> > > The authtag size can be up to 16 bytes, so I guess I can allocate 80 > bytes on stack and avoid the kzalloc(). > >> >>> + if (!resp) >>> + return -ENOMEM; >>> + >>> + /* Issue the command to get the attestation report */ >>> + rc = handle_guest_request(snp_dev, req.msg_version, >>> SNP_MSG_KEY_REQ, >>> + &req.data, sizeof(req.data), resp->data, resp_len, >>> + &arg->fw_err); >>> + if (rc) >>> + goto e_free; >>> + >>> + /* Copy the response payload to userspace */ >>> + if (copy_to_user((void __user *)arg->resp_data, resp, >>> sizeof(*resp))) >>> + rc = -EFAULT; >>> + >>> +e_free: >>> + kfree(resp); >> >> Since resp contains key material, I think you should explicit_memzero() >> it before freeing, so the key bytes don't linger around in unused >> memory. I'm not sure if any copies are made inside the >> handle_guest_request call above; maybe zero these as well. >> > > I can do that, but I guess I am trying to find a reason for it. The resp > buffer is encrypted page, so, the key is protected from the hypervisor > access. Are you thinking about an attack within the VM guest OS ? > Yes, that's the concern, specifically with sensitive buffers (keys). You don't want many copies floating around in unused memory. -Dov
On Fri, Aug 20, 2021 at 10:19:32AM -0500, Brijesh Singh wrote: > +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_req 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 fileds passed in the key derivation request. > + > +On success, the snp_derived_key_resp.data will contains the derived key "will contain" > +value. > diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c > index d029a98ad088..621b1c5a9cfc 100644 > --- a/drivers/virt/coco/sevguest/sevguest.c > +++ b/drivers/virt/coco/sevguest/sevguest.c > @@ -303,6 +303,50 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_user_guest_reque > return rc; > } > > +static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *arg) > +{ > + struct snp_guest_crypto *crypto = snp_dev->crypto; > + struct snp_derived_key_resp *resp; > + struct snp_derived_key_req req; > + int rc, resp_len; > + > + 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; > + > + /* 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; > + resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); > + if (!resp) > + return -ENOMEM; > + > + /* Issue the command to get the attestation report */ > + rc = handle_guest_request(snp_dev, req.msg_version, SNP_MSG_KEY_REQ, > + &req.data, sizeof(req.data), resp->data, resp_len, > + &arg->fw_err); > + if (rc) > + goto e_free; > + > + /* Copy the response payload to userspace */ > + if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp))) > + rc = -EFAULT; > + > +e_free: > + kfree(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); > @@ -320,6 +364,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long > ret = get_report(snp_dev, &input); > break; > } > + case SNP_GET_DERIVED_KEY: { > + ret = get_derived_key(snp_dev, &input); > + break; > + } {} brackets are not needed. What, however, is bothering me more in this function is that you call the respective ioctl function which might fail, you do not look at the return value and copy_to_user() unconditionally. Looking at get_derived_key(), for example, if it returns after: if (!arg->req_data || !arg->resp_data) return -EINVAL; you will be copying the same thing back to the user, you copied in earlier. That doesn't make any sense to me. Thx.
On 9/8/21 9:00 AM, Borislav Petkov wrote: > On Fri, Aug 20, 2021 at 10:19:32AM -0500, Brijesh Singh wrote: >> +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_req 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 fileds passed in the key derivation request. >> + >> +On success, the snp_derived_key_resp.data will contains the derived key > > "will contain" Noted. >> + >> + /* Copy the request payload from the userspace */ > > "from userspace" Noted. >> + >> static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) >> { >> struct snp_guest_dev *snp_dev = to_snp_dev(file); >> @@ -320,6 +364,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long >> ret = get_report(snp_dev, &input); >> break; >> } >> + case SNP_GET_DERIVED_KEY: { >> + ret = get_derived_key(snp_dev, &input); >> + break; >> + } > > {} brackets are not needed. > > What, however, is bothering me more in this function is that you call > the respective ioctl function which might fail, you do not look at the > return value and copy_to_user() unconditionally. > > Looking at get_derived_key(), for example, if it returns after: > > if (!arg->req_data || !arg->resp_data) > return -EINVAL; > > you will be copying the same thing back to the user, you copied in > earlier. That doesn't make any sense to me. I will look into improving it to copy back to userspace only if there is firmware error. thanks
diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst index 52d5915037ef..25446670d816 100644 --- a/Documentation/virt/coco/sevguest.rst +++ b/Documentation/virt/coco/sevguest.rst @@ -67,3 +67,21 @@ provided by the SEV-SNP firmware to query the attestation report. On success, the snp_report_resp.data will contains the report. The report format is 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_req 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 fileds passed in the key derivation request. + +On success, the snp_derived_key_resp.data will contains the derived key +value. diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c index d029a98ad088..621b1c5a9cfc 100644 --- a/drivers/virt/coco/sevguest/sevguest.c +++ b/drivers/virt/coco/sevguest/sevguest.c @@ -303,6 +303,50 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_user_guest_reque return rc; } +static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *arg) +{ + struct snp_guest_crypto *crypto = snp_dev->crypto; + struct snp_derived_key_resp *resp; + struct snp_derived_key_req req; + int rc, resp_len; + + 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; + + /* 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; + resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); + if (!resp) + return -ENOMEM; + + /* Issue the command to get the attestation report */ + rc = handle_guest_request(snp_dev, req.msg_version, SNP_MSG_KEY_REQ, + &req.data, sizeof(req.data), resp->data, resp_len, + &arg->fw_err); + if (rc) + goto e_free; + + /* Copy the response payload to userspace */ + if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp))) + rc = -EFAULT; + +e_free: + kfree(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); @@ -320,6 +364,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long 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 e8cfd15133f3..621a9167df7a 100644 --- a/include/uapi/linux/sev-guest.h +++ b/include/uapi/linux/sev-guest.h @@ -36,9 +36,33 @@ struct snp_user_guest_request { __u64 fw_err; }; +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_req { + /* message version number (must be non-zero) */ + __u8 msg_version; + + struct __snp_derived_key_req data; +}; + +struct snp_derived_key_resp { + /* response data, see SEV-SNP spec for the format */ + __u8 data[64]; +}; + #define SNP_GUEST_REQ_IOC_TYPE 'S' /* Get SNP attestation report */ #define SNP_GET_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x0, struct snp_user_guest_request) +/* Get a derived key from the root */ +#define SNP_GET_DERIVED_KEY _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x1, struct snp_user_guest_request) + #endif /* __UAPI_LINUX_SEV_GUEST_H_ */
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 choose, such as a sealing key or communicating with the external entities. See SEV-SNP firmware spec for more information. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- Documentation/virt/coco/sevguest.rst | 18 ++++++++++ drivers/virt/coco/sevguest/sevguest.c | 48 +++++++++++++++++++++++++++ include/uapi/linux/sev-guest.h | 24 ++++++++++++++ 3 files changed, 90 insertions(+)