Re: [PATCH Part2 v5 17/45] crypto: ccp: Add the SNP_{SET,GET}_EXT_CONFIG command

[Brijesh Singh <brijesh.singh@amd.com>]

On 9/9/21 10:27 PM, Marc Orr wrote:
> `
>
> On Fri, Aug 20, 2021 at 9:00 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
>> 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 |  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`?

Ah good point, I should provide an option to query the length when
input.cert_address == 0. This will make it much cleaner that there are
two approaches to get the length.


>> +
>> +                       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?
psp_copy_user_blob() is a wrapper around memdup_user() -- which does
kmalloc() + copy_to_user(). The wrapper performs some additional checks
such as the blob size etc, we limit the blob size to 16K to avoid
copying a random large data from userspace.
>> +               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?

This is mainly to help in the cases where the provisioning tools may not
keep track of the programmed TCB version and would prefer to read the
TCB version before updating.