Re: [PATCH RFC v9 48/51] crypto: ccp: Add the SNP_{SET,GET}_EXT_CONFIG command

From: Alexey Kardashevskiy <aik@amd.com>
To: Michael Roth <michael.roth@amd.com>, kvm@vger.kernel.org
Cc: linux-coco@lists.linux.dev, linux-mm@kvack.org,
	linux-crypto@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, tglx@linutronix.de,
	mingo@redhat.com, jroedel@suse.de, thomas.lendacky@amd.com,
	hpa@zytor.com, ardb@kernel.org, pbonzini@redhat.com,
	seanjc@google.com, vkuznets@redhat.com, jmattson@google.com,
	luto@kernel.org, dave.hansen@linux.intel.com, slp@redhat.com,
	pgonda@google.com, peterz@infradead.org,
	srinivas.pandruvada@linux.intel.com, rientjes@google.com,
	dovmurik@linux.ibm.com, tobin@ibm.com, bp@alien8.de,
	vbabka@suse.cz, kirill@shutemov.name, ak@linux.intel.com,
	tony.luck@intel.com, marcorr@google.com,
	sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
	dgilbert@redhat.com, jarkko@kernel.org, ashish.kalra@amd.com,
	nikunj.dadhania@amd.com, liam.merwick@oracle.com,
	zhi.a.wang@intel.com, Brijesh Singh <brijesh.singh@amd.com>,
	Dionna Glaze <dionnaglaze@google.com>
Subject: Re: [PATCH RFC v9 48/51] crypto: ccp: Add the SNP_{SET,GET}_EXT_CONFIG command
Date: Tue, 13 Jun 2023 16:24:41 +1000	[thread overview]
Message-ID: <ec1880ea-4b81-faf8-054e-220d58ac9775@amd.com> (raw)
In-Reply-To: <20230612042559.375660-49-michael.roth@amd.com>

On 12/6/23 14:25, Michael Roth 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.
> 
> Co-developed-by: Alexey Kardashevskiy <aik@amd.com>
> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
> Co-developed-by: Dionna Glaze <dionnaglaze@google.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> [mdr: squash in doc patch from Dionna]
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>   Documentation/virt/coco/sev-guest.rst |  27 ++++
>   drivers/crypto/ccp/sev-dev.c          | 178 ++++++++++++++++++++++++++
>   drivers/crypto/ccp/sev-dev.h          |   2 +
>   include/linux/psp-sev.h               |  10 ++
>   include/uapi/linux/psp-sev.h          |  17 +++
>   5 files changed, 234 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 b8e8c4da4025..175c24163ba0 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1491,6 +1491,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 */
> +	sev_snp_certs_put(sev->snp_certs);
> +	sev->snp_certs = NULL;
> +
>   	wbinvd_on_all_cpus();
>   
>   retry:
> @@ -1829,6 +1833,126 @@ 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;

input = {0} would do as well as the memset() below but shorter.

> +	struct sev_snp_certs *snp_certs;
> +	int ret;
> +
> +	if (!sev->snp_initialized || !argp->data)
> +		return -EINVAL;
> +
> +	memset(&input, 0, sizeof(input));

but this memset() seems useless anyway because of copy_from_user() below.

> +
> +	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;
> +	}
> +
> +	snp_certs = sev_snp_certs_get(sev->snp_certs);
> +
> +	/* Copy the extended certs programmed through the SNP_SET_CONFIG */
> +	if (input.certs_address && snp_certs) {
> +		if (input.certs_len < snp_certs->len) {
> +			/* Return the certs length to userspace */
> +			input.certs_len = snp_certs->len;
> +
> +			ret = -EIO;
> +			goto e_done;
> +		}
> +
> +		if (copy_to_user((void * __user)input.certs_address,
> +				 snp_certs->data, snp_certs->len)) {
> +			ret = -EFAULT;
> +			goto put_exit;
> +		}
> +	}
> +
> +	ret = 0;
> +
> +e_done:
> +	if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
> +		ret = -EFAULT;
> +
> +put_exit:
> +	sev_snp_certs_put(snp_certs);
> +
> +	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;
> +	struct sev_snp_certs *snp_certs = NULL;
> +	void *certs = NULL;
> +	int ret = 0;

This '0' is not used below - it is always overwritten when "goto e_free" 
and the good exit is "return 0" and not "return ret". I'd suggest either 
not initializing it and letting gcc barf when some future change uses 
it, or initialize to something like -EFAULT.

> +
> +	if (!sev->snp_initialized || !argp->data)
> +		return -EINVAL;
> +
> +	if (!writable)
> +		return -EPERM;
> +
> +	memset(&input, 0, sizeof(input));

same here.

> +
> +	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));

and here.

> +		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 (input.certs_len) {
> +		snp_certs = sev_snp_certs_new(certs, input.certs_len);
> +		if (!snp_certs) {
> +			ret = -ENOMEM;
> +			goto e_free;
> +		}
> +	}
> +
> +	sev_snp_certs_put(sev->snp_certs);
> +	sev->snp_certs = snp_certs;
> +
> +	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;
> @@ -1883,6 +2007,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;
> @@ -1931,6 +2061,54 @@ int sev_guest_df_flush(int *error)
>   }
>   EXPORT_SYMBOL_GPL(sev_guest_df_flush);
>   
> +static void sev_snp_certs_release(struct kref *kref)
> +{
> +	struct sev_snp_certs *certs = container_of(kref, struct sev_snp_certs, kref);
> +
> +	kfree(certs->data);
> +	kfree(certs);
> +}
> +
> +struct sev_snp_certs *sev_snp_certs_new(void *data, u32 len)
> +{
> +	struct sev_snp_certs *certs;
> +
> +	if (!len || !data)
> +		return NULL;
> +
> +	certs = kzalloc(sizeof(*certs), GFP_KERNEL);
> +	if (!certs)
> +		return NULL;
> +
> +	certs->data = data;
> +	certs->len = len;
> +	kref_init(&certs->kref);
> +
> +	return certs;
> +}
> +EXPORT_SYMBOL_GPL(sev_snp_certs_new);
> +
> +struct sev_snp_certs *sev_snp_certs_get(struct sev_snp_certs *certs)
> +{
> +	if (!certs)
> +		return NULL;
> +
> +	if (!kref_get_unless_zero(&certs->kref))
> +		return NULL;
> +
> +	return certs;
> +}
> +EXPORT_SYMBOL_GPL(sev_snp_certs_get);
> +
> +void sev_snp_certs_put(struct sev_snp_certs *certs)
> +{
> +	if (!certs)
> +		return;
> +
> +	kref_put(&certs->kref, sev_snp_certs_release);
> +}
> +EXPORT_SYMBOL_GPL(sev_snp_certs_put);
> +
>   static void sev_exit(struct kref *ref)
>   {
>   	misc_deregister(&misc_dev->misc);
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 19d79f9d4212..22374f3d3e2e 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -66,6 +66,8 @@ struct sev_device {
>   
>   	bool snp_initialized;
>   	struct snp_host_map snp_host_map[MAX_SNP_HOST_MAP_BUFS];
> +	struct sev_snp_certs *snp_certs;
> +	struct sev_user_data_snp_config snp_config;
>   };
>   
>   int sev_dev_init(struct psp_device *psp);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 5ae61de96e44..2191d8b5423a 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -24,6 +24,16 @@
>   
>   #define SEV_FW_BLOB_MAX_SIZE	0x4000	/* 16KB */
>   
> +struct sev_snp_certs {
> +	void *data;
> +	u32 len;
> +	struct kref kref;
> +};
> +
> +struct sev_snp_certs *sev_snp_certs_new(void *data, u32 len);
> +struct sev_snp_certs *sev_snp_certs_get(struct sev_snp_certs *certs);
> +void sev_snp_certs_put(struct sev_snp_certs *certs);
> +
>   /**
>    * SEV platform state
>    */
> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> index 4dc6a3e7b3d5..d1e6a0615546 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,
>   };
> @@ -201,6 +203,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 */
> +};

__packed or padding missing (there are other places like btw, I remember 
seeing quite a few of those). Thanks,

> +
>   /**
>    * struct sev_issue_cmd - SEV ioctl parameters
>    *

-- 
Alexey