Re: [PATCH Part2 v5 17/45] crypto: ccp: Add the SNP_{SET,GET}_EXT_CONFIG command
From: Marc Orr <hidden>
Date: 2021-09-10 03:28:18
Also in:
kvm, linux-coco, linux-mm, lkml
` On Fri, Aug 20, 2021 at 9:00 AM Brijesh Singh [off-list ref] wrote:
quoted hunk ↗ jump to hunk
The SEV-SNP firmware provides the SNP_CONFIG command used to set the system-wide configuration value for SNP guests. The information includes the TCB version string to be reported in guest attestation reports. Version 2 of the GHCB specification adds an NAE (SNP extended guest request) that a guest can use to query the reports that include additional certificates. In both cases, userspace provided additional data is included in the attestation reports. The userspace will use the SNP_SET_EXT_CONFIG command to give the certificate blob and the reported TCB version string at once. Note that the specification defines certificate blob with a specific GUID format; the userspace is responsible for building the proper certificate blob. The ioctl treats it an opaque blob. While it is not defined in the spec, but let's add SNP_GET_EXT_CONFIG command that can be used to obtain the data programmed through the SNP_SET_EXT_CONFIG. Signed-off-by: Brijesh Singh <redacted> --- Documentation/virt/coco/sevguest.rst | 28 +++++++ drivers/crypto/ccp/sev-dev.c | 115 +++++++++++++++++++++++++++ drivers/crypto/ccp/sev-dev.h | 3 + include/uapi/linux/psp-sev.h | 17 ++++ 4 files changed, 163 insertions(+)diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst index 7c51da010039..64a1b5167b33 100644 --- a/Documentation/virt/coco/sevguest.rst +++ b/Documentation/virt/coco/sevguest.rst@@ -134,3 +134,31 @@ See GHCB specification for further detail on how to parse the certificate blob. The SNP_PLATFORM_STATUS command is used to query the SNP platform status. The status includes API major, minor version and more. See the SEV-SNP specification for further details. + +2.4 SNP_SET_EXT_CONFIG +---------------------- +:Technology: sev-snp +:Type: hypervisor ioctl cmd +:Parameters (in): struct sev_data_snp_ext_config +:Returns (out): 0 on success, -negative on error + +The SNP_SET_EXT_CONFIG is used to set the system-wide configuration such as +reported TCB version in the attestation report. The command is similar to +SNP_CONFIG command defined in the SEV-SNP spec. The main difference is the +command also accepts an additional certificate blob defined in the GHCB +specification. + +If the certs_address is zero, then previous certificate blob will deleted. +For more information on the certificate blob layout, see the GHCB spec +(extended guest request message). + + +2.4 SNP_GET_EXT_CONFIG +---------------------- +:Technology: sev-snp +:Type: hypervisor ioctl cmd +:Parameters (in): struct sev_data_snp_ext_config +:Returns (out): 0 on success, -negative on error + +The SNP_SET_EXT_CONFIG is used to query the system-wide configuration set +through the SNP_SET_EXT_CONFIG.diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 16c6df5d412c..9ba194acbe85 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c@@ -1132,6 +1132,10 @@ static int __sev_snp_shutdown_locked(int *error) if (!sev->snp_inited) return 0; + /* Free the memory used for caching the certificate data */ + kfree(sev->snp_certs_data); + sev->snp_certs_data = NULL; + /* SHUTDOWN requires the DF_FLUSH */ wbinvd_on_all_cpus(); __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, NULL);@@ -1436,6 +1440,111 @@ static int sev_ioctl_snp_platform_status(struct sev_issue_cmd *argp) return ret; } +static int sev_ioctl_snp_get_config(struct sev_issue_cmd *argp) +{ + struct sev_device *sev = psp_master->sev_data; + struct sev_user_data_ext_snp_config input; + int ret; + + if (!sev->snp_inited || !argp->data) + return -EINVAL; + + if (copy_from_user(&input, (void __user *)argp->data, sizeof(input))) + return -EFAULT; + + /* Copy the TCB version programmed through the SET_CONFIG to userspace */ + if (input.config_address) { + if (copy_to_user((void * __user)input.config_address, + &sev->snp_config, sizeof(struct sev_user_data_snp_config))) + return -EFAULT; + } + + /* Copy the extended certs programmed through the SNP_SET_CONFIG */ + if (input.certs_address && sev->snp_certs_data) { + if (input.certs_len < sev->snp_certs_len) { + /* Return the certs length to userspace */ + input.certs_len = sev->snp_certs_len;
This API to retrieve the length of the certs seems pretty odd. We only return the length if the input.certs_address is non-NULL. But if we know the length how did we allocate an address to write to `input.certs_address`?
+
+ ret = -ENOSR;
+ goto e_done;
+ }
+
+ if (copy_to_user((void * __user)input.certs_address,
+ sev->snp_certs_data, sev->snp_certs_len))
+ return -EFAULT;
+ }
+
+ ret = 0;
+
+e_done:
+ if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
+ ret = -EFAULT;
+
+ return ret;
+}
+
+static int sev_ioctl_snp_set_config(struct sev_issue_cmd *argp, bool writable)
+{
+ struct sev_device *sev = psp_master->sev_data;
+ struct sev_user_data_ext_snp_config input;
+ struct sev_user_data_snp_config config;
+ void *certs = NULL;
+ int ret = 0;
+
+ if (!sev->snp_inited || !argp->data)
+ return -EINVAL;
+
+ if (!writable)
+ return -EPERM;
+
+ if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
+ return -EFAULT;
+
+ /* Copy the certs from userspace */
+ if (input.certs_address) {
+ if (!input.certs_len || !IS_ALIGNED(input.certs_len, PAGE_SIZE))
+ return -EINVAL;
+
+ certs = psp_copy_user_blob(input.certs_address, input.certs_len);Is `psp_copy_user_blob()` implemented in this patch series? When I searched through the patches, I only found an implementation that always returns an error. But maybe I missed the implementation? Also, out of curiosity, any reason we cannot use copy_from_user here?
quoted hunk ↗ jump to hunk
+ if (IS_ERR(certs)) + return PTR_ERR(certs); + } + + /* Issue the PSP command to update the TCB version using the SNP_CONFIG. */ + if (input.config_address) { + if (copy_from_user(&config, + (void __user *)input.config_address, sizeof(config))) { + ret = -EFAULT; + goto e_free; + } + + ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error); + if (ret) + goto e_free; + + memcpy(&sev->snp_config, &config, sizeof(config)); + } + + /* + * If the new certs are passed then cache it else free the old certs. + */ + if (certs) { + kfree(sev->snp_certs_data); + sev->snp_certs_data = certs; + sev->snp_certs_len = input.certs_len; + } else { + kfree(sev->snp_certs_data); + sev->snp_certs_data = NULL; + sev->snp_certs_len = 0; + } + + return 0; + +e_free: + kfree(certs); + return ret; +} + static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) { void __user *argp = (void __user *)arg;@@ -1490,6 +1599,12 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) case SNP_PLATFORM_STATUS: ret = sev_ioctl_snp_platform_status(&input); break; + case SNP_SET_EXT_CONFIG: + ret = sev_ioctl_snp_set_config(&input, writable); + break; + case SNP_GET_EXT_CONFIG: + ret = sev_ioctl_snp_get_config(&input); + break;
What is the intended use of `SNP_GET_EXT_CONFIG`. Yes, I get that it returns the "EXT config" previously set via `SNP_SET_EXT_CONFIG`. But presumably the caller can keep track of what it's previously passed to `SNP_SET_EXT_CONFIG`. Does it really need to call into the kernel to get these certs?
quoted hunk ↗ jump to hunk
default: ret = -EINVAL; goto out;diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h index fe5d7a3ebace..d2fe1706311a 100644 --- a/drivers/crypto/ccp/sev-dev.h +++ b/drivers/crypto/ccp/sev-dev.h@@ -66,6 +66,9 @@ struct sev_device { bool snp_inited; struct snp_host_map snp_host_map[MAX_SNP_HOST_MAP_BUFS]; + void *snp_certs_data; + u32 snp_certs_len; + struct sev_user_data_snp_config snp_config; }; int sev_dev_init(struct psp_device *psp);diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h index ffd60e8b0a31..60e7a8d1a18e 100644 --- a/include/uapi/linux/psp-sev.h +++ b/include/uapi/linux/psp-sev.h@@ -29,6 +29,8 @@ enum { SEV_GET_ID, /* This command is deprecated, use SEV_GET_ID2 */ SEV_GET_ID2, SNP_PLATFORM_STATUS, + SNP_SET_EXT_CONFIG, + SNP_GET_EXT_CONFIG, SEV_MAX, };@@ -190,6 +192,21 @@ struct sev_user_data_snp_config { __u8 rsvd[52]; } __packed; +/** + * struct sev_data_snp_ext_config - system wide configuration value for SNP. + * + * @config_address: address of the struct sev_user_data_snp_config or 0 when + * reported_tcb does not need to be updated. + * @certs_address: address of extended guest request certificate chain or + * 0 when previous certificate should be removed on SNP_SET_EXT_CONFIG. + * @certs_len: length of the certs + */ +struct sev_user_data_ext_snp_config { + __u64 config_address; /* In */ + __u64 certs_address; /* In */ + __u32 certs_len; /* In */ +}; + /** * struct sev_issue_cmd - SEV ioctl parameters * --2.17.1