Re: [PATCH RFC v8 27/56] crypto: ccp: Add the SNP_{SET,GET}_EXT_CONFIG command

["Kalra, Ashish" <ashish.kalra@xxxxxxx>]

On 2/22/2023 6:32 AM, Zhi Wang wrote:
> On Mon, 20 Feb 2023 12:38:18 -0600
> Michael Roth <michael.roth@xxxxxxx> wrote:
>
> > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> >   Documentation/virt/coco/sev-guest.rst |  27 ++++++
> >   drivers/crypto/ccp/sev-dev.c          | 123 ++++++++++++++++++++++++++
> >   drivers/crypto/ccp/sev-dev.h          |   4 +
> >   include/uapi/linux/psp-sev.h          |  17 ++++
> >   4 files changed, 171 insertions(+)
> >
> > diff --git a/Documentation/virt/coco/sev-guest.rst b/Documentation/virt/coco/sev-guest.rst
> > index 11ea67c944df..6cad4226c348 100644
> > --- a/Documentation/virt/coco/sev-guest.rst
> > +++ b/Documentation/virt/coco/sev-guest.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 the 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_GET_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 65e13a562f3b..b56b00ca2cd4 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -1481,6 +1481,10 @@ static int __sev_snp_shutdown_locked(int *error)
> >   	data.length = sizeof(data);
> >   	data.iommu_snp_shutdown = 1;
> >   
> > +	/* Free the memory used for caching the certificate data */
> > +	kfree(sev->snp_certs_data);
> > +	sev->snp_certs_data = NULL;
> > +
> >   	wbinvd_on_all_cpus();
> >   
> >   retry:
> > @@ -1793,6 +1797,118 @@ 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_initialized || !argp->data)
> > +		return -EINVAL;
> > +
> > +	memset(&input, 0, sizeof(input));
> > +
> > +	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;
> > +		}
> > +
>
> What about if input.certs_len > sev->snp_certs_len? Is it possbile for the
> userspace to know the length of data in the buffer? (I guess it might be able
> to know the certs len through the blob data, but a comment here would be nice)

If userspace provides an input buffer/length smaller then snp_certs_len, 
then the above returns the "required" certs length back to userspace.

And what is the issue if input.certs_len > sev->snp_certs_len, the 
buffer returned back to userspace is sev->snp_certs_len as below.

Thanks,
Ashish

> > +		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_initialized || !argp->data)
> > +		return -EINVAL;
> > +
> > +	if (!writable)
> > +		return -EPERM;
> > +
> > +	memset(&input, 0, sizeof(input));
> > +
> > +	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) {
> > +		memset(&config, 0, sizeof(config));
> > +		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.
> > +	 */
> > +	mutex_lock(&sev->snp_certs_lock);
> > +	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;
> > +	}
> > +	mutex_unlock(&sev->snp_certs_lock);
> > +
> > +	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;
> > @@ -1847,6 +1963,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;
> > @@ -1962,6 +2084,7 @@ int sev_dev_init(struct psp_device *psp)
> >   		goto e_sev;
> >   
> >   	sev->cmd_buf_backup = (uint8_t *)sev->cmd_buf + PAGE_SIZE;
> > +	mutex_init(&sev->snp_certs_lock);
> >   
> >   	psp->sev_data = sev;
> >   
> > diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> > index 19d79f9d4212..41d5353d5bab 100644
> > --- a/drivers/crypto/ccp/sev-dev.h
> > +++ b/drivers/crypto/ccp/sev-dev.h
> > @@ -66,6 +66,10 @@ struct sev_device {
> >   
> >   	bool snp_initialized;
> >   	struct snp_host_map snp_host_map[MAX_SNP_HOST_MAP_BUFS];
> > +	void *snp_certs_data;
> > +	u32 snp_certs_len;
> > +	struct mutex snp_certs_lock;
> > +	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 5adfaea7df97..c20d37586d21 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,
> >   };
> > @@ -192,6 +194,21 @@ struct sev_user_data_snp_config {
> >   	__u8 rsvd1[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
> >    *