| Message ID | d325cb5d7961f015400999dda7ee8e08e4ca2ec6.1655761627.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) | expand |
On Mon, Jun 20, 2022 at 5:06 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote: > > From: Brijesh Singh <brijesh.singh@amd.com> > > 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 <brijesh.singh@amd.com> > --- > Documentation/virt/coco/sevguest.rst | 27 +++++++ > drivers/crypto/ccp/sev-dev.c | 115 +++++++++++++++++++++++++++ > drivers/crypto/ccp/sev-dev.h | 3 + > include/uapi/linux/psp-sev.h | 17 ++++ > 4 files changed, 162 insertions(+) > > diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst > index 11ea67c944df..3014de47e4ce 100644 > --- a/Documentation/virt/coco/sevguest.rst > +++ b/Documentation/virt/coco/sevguest.rst > @@ -145,6 +145,33 @@ 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.5 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. ... then the previous certificate blob will be deleted. > +For more information on the certificate blob layout, see the GHCB spec > +(extended guest request message). > + > +2.6 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. > + > 3. SEV-SNP CPUID Enforcement > ============================ > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index b9b6fab31a82..97b479d5aa86 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1312,6 +1312,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); > @@ -1616,6 +1620,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; Lets memset |input| to zero to avoid leaking kernel memory, see "crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak" > + 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; > + > + 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); I see that psp_copy_user_blob() uses memdup_user() which tracks the allocated memory to GFP_USER. Given this memory is long lived and now belongs to the PSP driver in perpetuity, should this be tracked with GFP_KERNEL? > + 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; > + } Do we need another lock here? When I look at 18/49 it seems like snp_guest_ext_guest_request() it seems like we have a race for |sev->snp_certs_data| > + > + 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; > @@ -1670,6 +1779,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; > 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; Since this gets copy_to_user'd can we memset this to 0 to prevent leaking kernel uninitialized memory? Similar to recent patches with kzalloc and __GPF_ZERO usage. > }; > > 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.25.1 >
[AMD Official Use Only - General] Hello Peter, >> +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; >Lets memset |input| to zero to avoid leaking kernel memory, see >"crypto: ccp - Use kzalloc for sev ioctl interfaces to prevent kernel memory leak" Yes. >> +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); >I see that psp_copy_user_blob() uses memdup_user() which tracks the allocated memory to GFP_USER. Given this memory is long lived and now belongs to the PSP driver in perpetuity, should this be tracked with GFP_KERNEL? But we need to copy from user space address, so what is the alternative here ? >> + /* >> + * 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; >> + } >Do we need another lock here? When I look at 18/49 it seems like >snp_guest_ext_guest_request() it seems like we have a race for >|sev->snp_certs_data| The certificate blob in snp_guest_ext_guest_request() will depend on the certificate blob provided here by SNP_SET_EXT_CONFIG. There might be a potential race with the SNP extended guest request NAE, let me have a look at it. >> 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; >Since this gets copy_to_user'd can we memset this to 0 to prevent leaking kernel uninitialized memory? Similar to recent patches with kzalloc and __GPF_ZERO usage. Yes. Thanks, Ashish
On Mon, Jun 20, 2022 at 11:05:50PM +0000, Ashish Kalra wrote: > From: Brijesh Singh <brijesh.singh@amd.com> > > 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. Neither in the commit message nor in the documentation is GHCB open coded. > 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 <brijesh.singh@amd.com> > --- > Documentation/virt/coco/sevguest.rst | 27 +++++++ > drivers/crypto/ccp/sev-dev.c | 115 +++++++++++++++++++++++++++ > drivers/crypto/ccp/sev-dev.h | 3 + > include/uapi/linux/psp-sev.h | 17 ++++ > 4 files changed, 162 insertions(+) > > diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst > index 11ea67c944df..3014de47e4ce 100644 > --- a/Documentation/virt/coco/sevguest.rst > +++ b/Documentation/virt/coco/sevguest.rst > @@ -145,6 +145,33 @@ 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.5 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.6 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. > + > 3. SEV-SNP CPUID Enforcement > ============================ > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index b9b6fab31a82..97b479d5aa86 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1312,6 +1312,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); > @@ -1616,6 +1620,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; > + > + 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); > + 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))) { You can put this into a single line, and it's still less than 100 characters: 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. > + */ kfree(sev->snp_certs_data); > + if (certs) { > + kfree(sev->snp_certs_data); Remove kfree(). > + sev->snp_certs_data = certs; > + sev->snp_certs_len = input.certs_len; > + } else { > + kfree(sev->snp_certs_data); Remove kfree(). > + 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; > @@ -1670,6 +1779,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; > 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.25.1 > BR, Jarkko
To preface, I don't want to delay this patch set, only have the conversation at the most appropriate place. > > > 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. > The system-wide aspect of this makes me wonder if we can also have a VM instance-specific extension. This is important for the use case that we may see secure boot variables included in the launch measurement, making offline signing of the UEFI image impossible. We can't sign the cross-product of all UEFI builds and every user's EFI variables. We'd like to include an instance-specific certificate that specifies the platform-endorsed golden measurement of the UEFI. An alternative that doesn't require a change to the kernel is to just make this certificate fetchable from a FAMILY_ID-keyed, predetermined URL prefix + IMAGE_ID + '.crt', but this requires a download (and continuous hosting) to do something as routine as collecting an attestation report. It's up to the upstream community to determine if that is an acceptable cost to keep the complexity of a certificate table merge operation out of the kernel. The SNP API specification gives an interpretation to the data blob here as a table of GUID/offset pairs followed by data blobs that presumably are at the appropriate offsets into the data pages. The spec allows for the host to add any number of GUID/offset pairs it wants, with 3 specific GUIDs recommended for the AMD PSP certificate chain. The snp_guest_ext_guest_request function in ccp is what passes back the certificate data that was previously stored, so I'm wondering if it can take an extra (pointer,len) pair of VM instance certificate data to merge with the host certificate data before returning to the guest. The new required length is the sum total of both the header certs and instance certs. The operation to copy the data is no longer a memcpy but a header merge that tracks the offset shifts caused by a larger header and other certificates in the remaining data pages. I can propose my own patch on top of this v6 patch set that adds a KVM ioctl like KVM_{GET,SET}_INSTANCE_SNP_EXT_CONFIG and then pass along the stored certificate blob in the request call. I'd prefer to have the design agreed upon upfront though.
On 8/8/22 14:27, Dionna Amalie Glaze wrote: > To preface, I don't want to delay this patch set, only have the > conversation at the most appropriate place. > >> >>> 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. >> > > The system-wide aspect of this makes me wonder if we can also have a > VM instance-specific extension. This is important for the use case > that we may see secure boot variables included in the launch > measurement, making offline signing of the UEFI image impossible. We > can't sign the cross-product of all UEFI builds and every user's EFI > variables. We'd like to include an instance-specific certificate that > specifies the platform-endorsed golden measurement of the UEFI. > > An alternative that doesn't require a change to the kernel is to just > make this certificate fetchable from a FAMILY_ID-keyed, predetermined > URL prefix + IMAGE_ID + '.crt', but this requires a download (and > continuous hosting) to do something as routine as collecting an > attestation report. It's up to the upstream community to determine if > that is an acceptable cost to keep the complexity of a certificate > table merge operation out of the kernel. > > The SNP API specification gives an interpretation to the data blob That's the GHCB specification, not the SNP API. > here as a table of GUID/offset pairs followed by data blobs that > presumably are at the appropriate offsets into the data pages. The > spec allows for the host to add any number of GUID/offset pairs it > wants, with 3 specific GUIDs recommended for the AMD PSP certificate > chain. > > The snp_guest_ext_guest_request function in ccp is what passes back > the certificate data that was previously stored, so I'm wondering if > it can take an extra (pointer,len) pair of VM instance certificate > data to merge with the host certificate data before returning to the > guest. The new required length is the sum total of both the header > certs and instance certs. The operation to copy the data is no longer > a memcpy but a header merge that tracks the offset shifts caused by a > larger header and other certificates in the remaining data pages. > > I can propose my own patch on top of this v6 patch set that adds a KVM > ioctl like KVM_{GET,SET}_INSTANCE_SNP_EXT_CONFIG and then pass along Would it be burden to supply all the certificates, both system and per-VM, in this KVM call? On the SNP Extended Guest Request, the hypervisor could just check if there is a per-VM blob and return that or else return the system-wide blob (if present). Thanks, Tom > the stored certificate blob in the request call. I'd prefer to have > the design agreed upon upfront though. >
> Would it be burden to supply all the certificates, both system and per-VM, > in this KVM call? On the SNP Extended Guest Request, the hypervisor could > just check if there is a per-VM blob and return that or else return the > system-wide blob (if present). > I think that's fine by me. We can use SNP_GET_EXT_CONFIG, merge in user space, and create an instance override with a KVM ioctl without touching ccp.
diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst index 11ea67c944df..3014de47e4ce 100644 --- a/Documentation/virt/coco/sevguest.rst +++ b/Documentation/virt/coco/sevguest.rst @@ -145,6 +145,33 @@ 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.5 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.6 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. + 3. SEV-SNP CPUID Enforcement ============================ diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index b9b6fab31a82..97b479d5aa86 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1312,6 +1312,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); @@ -1616,6 +1620,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; + + 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); + 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; @@ -1670,6 +1779,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; 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 *