[Part2,v6,17/49] crypto: ccp: Add the SNP_{SET,GET}_EXT_CONFIG command

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

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

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.