Thread (239 messages) 239 messages, 19 authors, 2022-09-19

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help