Re: [PATCH v2 25/25] crypto: ccp: Add the SNP_SET_CONFIG command

[Michael Roth]

On Mon, Jan 29, 2024 at 07:18:54PM +0000, Liam Merwick wrote:
> On 26/01/2024 04:11, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> > 
> > The SEV-SNP firmware provides the SNP_CONFIG command used to set various
> > system-wide configuration values for SNP guests, such as the reported
> > TCB version used when signing guest attestation reports. Add an
> > interface to set this via userspace.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > Co-developed-by: Alexey Kardashevskiy <aik@xxxxxxx>
> > Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
> > Co-developed-by: Dionna Glaze <dionnaglaze@xxxxxxxxxx>
> > Signed-off-by: Dionna Glaze <dionnaglaze@xxxxxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > [mdr: squash in doc patch from Dionna, drop extended request/certificate
> >   handling and simplify this to a simple wrapper around SNP_CONFIG fw
> >   cmd]
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> >   Documentation/virt/coco/sev-guest.rst | 13 +++++++++++++
> >   drivers/crypto/ccp/sev-dev.c          | 20 ++++++++++++++++++++
> >   include/uapi/linux/psp-sev.h          |  1 +
> >   3 files changed, 34 insertions(+)
> > 
> > diff --git a/Documentation/virt/coco/sev-guest.rst b/Documentation/virt/coco/sev-guest.rst
> > index 007ae828aa2a..14c9de997b7d 100644
> > --- a/Documentation/virt/coco/sev-guest.rst
> > +++ b/Documentation/virt/coco/sev-guest.rst
> > @@ -162,6 +162,19 @@ SEV-SNP firmware SNP_COMMIT command. This prevents roll-back to a previously
> >   committed firmware version. This will also update the reported TCB to match
> >   that of the currently installed firmware.
> >   
> > +2.6 SNP_SET_CONFIG
> > +------------------
> > +:Technology: sev-snp
> > +:Type: hypervisor ioctl cmd
> > +:Parameters (in): struct sev_user_data_snp_config
> > +:Returns (out): 0 on success, -negative on error
> > +
> > +SNP_SET_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 current values of
> > +the firmware parameters affected by this command can be queried via
> > +SNP_PLATFORM_STATUS.
> > +
> >   3. SEV-SNP CPUID Enforcement
> >   ============================
> >   
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 73ace4064e5a..398ae932aa0b 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -1982,6 +1982,23 @@ static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp)
> >   	return __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
> >   }
> >   
> > +static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable)
> > +{
> > +	struct sev_device *sev = psp_master->sev_data;
> 
> Should this check that psp_master is not NULL? Like the fix for
> https://lore.kernel.org/all/20240125231253.3122579-1-kim.phillips@xxxxxxx/

It wouldn't hurt, but sev_ioctl() will check for it before getting here,
so I don't think it's a reachable condition.

There's a number of spots where existing SEV/legacy code relies on
psp_pci_init() always setting psp_master before sev_pci_init(). Kim's
patch fixes 1 exception that's reachable if you synthetically force a
driver remove via DEBUG_TEST_DRIVER_REMOVE before the psp_pci_init()
call is made during probe, which generally wouldn't happen in practice.

But there may be other cases like the one Kim hit where "not work"
isn't handled gracefully. Fortunately the other obvious spot,
sev_do_cmd(), also checks for !psp_master, so that covers most of the
internal users of sev-dev.c.

But a general cleanup to introduce a helper like get_sev_device() that
handles !psp_master might be a nice follow-up to make things more
robust.

-Mike

> 
> Otherwise,
> Reviewed-by: Liam Merwick <liam.merwick@xxxxxxxxxx>
> 
> > +	struct sev_user_data_snp_config config;
> > +
> > +	if (!sev->snp_initialized || !argp->data)
> > +		return -EINVAL;
> > +
> > +	if (!writable)
> > +		return -EPERM;
> > +
> > +	if (copy_from_user(&config, (void __user *)argp->data, sizeof(config)))
> > +		return -EFAULT;
> > +
> > +	return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
> > +}
> > +
> >   static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> >   {
> >   	void __user *argp = (void __user *)arg;
> > @@ -2039,6 +2056,9 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> >   	case SNP_COMMIT:
> >   		ret = sev_ioctl_do_snp_commit(&input);
> >   		break;
> > +	case SNP_SET_CONFIG:
> > +		ret = sev_ioctl_do_snp_set_config(&input, writable);
> > +		break;
> >   	default:
> >   		ret = -EINVAL;
> >   		goto out;
> > diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> > index 35c207664e95..b7a2c2ee35b7 100644
> > --- a/include/uapi/linux/psp-sev.h
> > +++ b/include/uapi/linux/psp-sev.h
> > @@ -30,6 +30,7 @@ enum {
> >   	SEV_GET_ID2,
> >   	SNP_PLATFORM_STATUS,
> >   	SNP_COMMIT,
> > +	SNP_SET_CONFIG,
> >   
> >   	SEV_MAX,
> >   };
>