Message-ID: <CAAH4kHaM6BDD3Ry5KQJn0rVi7m+FCy2auQPnFNSxnMMeLziGhQ@mail.gmail.com>
Date: Thu, 20 Feb 2025 14:39:51 -0800
From: Dionna Amalie Glaze <dionnaglaze@...gle.com>
To: "Kalra, Ashish" <ashish.kalra@....com>
Cc: seanjc@...gle.com, pbonzini@...hat.com, tglx@...utronix.de, 
	mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, 
	hpa@...or.com, thomas.lendacky@....com, john.allen@....com, 
	herbert@...dor.apana.org.au, michael.roth@....com, nikunj@....com, 
	ardb@...nel.org, kevinloughlin@...gle.com, Neeraj.Upadhyay@....com, 
	aik@....com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-crypto@...r.kernel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH v4 2/7] crypto: ccp: Ensure implicit SEV/SNP init and
 shutdown in ioctls

On Thu, Feb 20, 2025 at 2:18 PM Kalra, Ashish <ashish.kalra@....com> wrote:
>
> On 2/20/2025 3:37 PM, Dionna Amalie Glaze wrote:
> > On Thu, Feb 20, 2025 at 12:07 PM Kalra, Ashish <ashish.kalra@....com> wrote:
> >>
> >> Hello Dionna,
> >>
> >> On 2/20/2025 10:44 AM, Dionna Amalie Glaze wrote:
> >>> On Wed, Feb 19, 2025 at 12:53 PM Ashish Kalra <Ashish.Kalra@....com> wrote:
> >>>>
> >>>> From: Ashish Kalra <ashish.kalra@....com>
> >>>>
> >>>> Modify the behavior of implicit SEV initialization in some of the
> >>>> SEV ioctls to do both SEV initialization and shutdown and add
> >>>> implicit SNP initialization and shutdown to some of the SNP ioctls
> >>>> so that the change of SEV/SNP platform initialization not being
> >>>> done during PSP driver probe time does not break userspace tools
> >>>> such as sevtool, etc.
> >>>>
> >>>> Prior to this patch, SEV has always been initialized before these
> >>>> ioctls as SEV initialization is done as part of PSP module probe,
> >>>> but now with SEV initialization being moved to KVM module load instead
> >>>> of PSP driver probe, the implied SEV INIT actually makes sense and gets
> >>>> used and additionally to maintain SEV platform state consistency
> >>>> before and after the ioctl SEV shutdown needs to be done after the
> >>>> firmware call.
> >>>>
> >>>> It is important to do SEV Shutdown here with the SEV/SNP initialization
> >>>> moving to KVM, an implicit SEV INIT here as part of the SEV ioctls not
> >>>> followed with SEV Shutdown will cause SEV to remain in INIT state and
> >>>> then a future SNP INIT in KVM module load will fail.
> >>>>
> >>>> Similarly, prior to this patch, SNP has always been initialized before
> >>>> these ioctls as SNP initialization is done as part of PSP module probe,
> >>>> therefore, to keep a consistent behavior, SNP init needs to be done
> >>>> here implicitly as part of these ioctls followed with SNP shutdown
> >>>> before returning from the ioctl to maintain the consistent platform
> >>>> state before and after the ioctl.
> >>>>
> >>>> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
> >>>> ---
> >>>>  drivers/crypto/ccp/sev-dev.c | 117 ++++++++++++++++++++++++++++-------
> >>>>  1 file changed, 93 insertions(+), 24 deletions(-)
> >>>>
> >>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> >>>> index 8f5c474b9d1c..b06f43eb18f7 100644
> >>>> --- a/drivers/crypto/ccp/sev-dev.c
> >>>> +++ b/drivers/crypto/ccp/sev-dev.c
> >>>> @@ -1461,7 +1461,8 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
> >>>>  static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
> >>>>  {
> >>>>         struct sev_device *sev = psp_master->sev_data;
> >>>> -       int rc;
> >>>> +       bool shutdown_required = false;
> >>>> +       int rc, error;
> >>>>
> >>>>         if (!writable)
> >>>>                 return -EPERM;
> >>>> @@ -1470,19 +1471,26 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
> >>>>                 rc = __sev_platform_init_locked(&argp->error);
> >>>>                 if (rc)
> >>>>                         return rc;
> >>>> +               shutdown_required = true;
> >>>>         }
> >>>>
> >>>> -       return __sev_do_cmd_locked(cmd, NULL, &argp->error);
> >>>> +       rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
> >>>> +
> >>>> +       if (shutdown_required)
> >>>> +               __sev_platform_shutdown_locked(&error);
> >>>
> >>> This error is discarded. Is that by design? If so, It'd be better to
> >>> call this ignored_error.
> >>>
> >>
> >> This is by design, we cannot overwrite the error for the original command being issued
> >> here which in this case is do_pek_pdh_gen, hence we use a local error for the shutdown command.
> >> And __sev_platform_shutdown_locked() has it's own error logging code, so it will be printing
> >> the error message for the shutdown command failure, so the shutdown error is not eventually
> >> being ignored, that error log will assist in any inconsistent SEV/SNP platform state and
> >> subsequent errors.
> >>
> >>>> +
> >>>> +       return rc;
> >>>>  }
> >>>>
> >>>>  static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
> >>>>  {
> >>>>         struct sev_device *sev = psp_master->sev_data;
> >>>>         struct sev_user_data_pek_csr input;
> >>>> +       bool shutdown_required = false;
> >>>>         struct sev_data_pek_csr data;
> >>>>         void __user *input_address;
> >>>>         void *blob = NULL;
> >>>> -       int ret;
> >>>> +       int ret, error;
> >>>>
> >>>>         if (!writable)
> >>>>                 return -EPERM;
> >>>> @@ -1513,6 +1521,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
> >>>>                 ret = __sev_platform_init_locked(&argp->error);
> >>>>                 if (ret)
> >>>>                         goto e_free_blob;
> >>>> +               shutdown_required = true;
> >>>>         }
> >>>>
> >>>>         ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
> >>>> @@ -1531,6 +1540,9 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
> >>>>         }
> >>>>
> >>>>  e_free_blob:
> >>>> +       if (shutdown_required)
> >>>> +               __sev_platform_shutdown_locked(&error);
> >>>
> >>> Another discarded error. This function is called in different
> >>> locations in sev-dev.c with and without checking the result, which
> >>> seems problematic.
> >>
> >> Not really, if shutdown fails for any reason, the error is printed.
> >> The return value here reflects the value of the original command/function.
> >> The command/ioctl could have succeeded but the shutdown failed, hence,
> >> shutdown error is printed, but the return value reflects that the ioctl succeeded.
> >>
> >> Additionally, in case of INIT before the command is issued, the command may
> >> have failed without the SEV state being in INIT state, hence the error for the
> >> INIT command failure is returned back from the ioctl.
> >>
> >>>
> >>>> +
> >>>>         kfree(blob);
> >>>>         return ret;
> >>>>  }
> >>>> @@ -1747,8 +1759,9 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
> >>>>         struct sev_device *sev = psp_master->sev_data;
> >>>>         struct sev_user_data_pek_cert_import input;
> >>>>         struct sev_data_pek_cert_import data;
> >>>> +       bool shutdown_required = false;
> >>>>         void *pek_blob, *oca_blob;
> >>>> -       int ret;
> >>>> +       int ret, error;
> >>>>
> >>>>         if (!writable)
> >>>>                 return -EPERM;
> >>>> @@ -1780,11 +1793,15 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
> >>>>                 ret = __sev_platform_init_locked(&argp->error);
> >>>>                 if (ret)
> >>>>                         goto e_free_oca;
> >>>> +               shutdown_required = true;
> >>>>         }
> >>>>
> >>>>         ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
> >>>>
> >>>>  e_free_oca:
> >>>> +       if (shutdown_required)
> >>>> +               __sev_platform_shutdown_locked(&error);
> >>>
> >>> Again.
> >>>
> >>>> +
> >>>>         kfree(oca_blob);
> >>>>  e_free_pek:
> >>>>         kfree(pek_blob);
> >>>> @@ -1901,17 +1918,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
> >>>>         struct sev_data_pdh_cert_export data;
> >>>>         void __user *input_cert_chain_address;
> >>>>         void __user *input_pdh_cert_address;
> >>>> -       int ret;
> >>>> -
> >>>> -       /* If platform is not in INIT state then transition it to INIT. */
> >>>> -       if (sev->state != SEV_STATE_INIT) {
> >>>> -               if (!writable)
> >>>> -                       return -EPERM;
> >>>> -
> >>>> -               ret = __sev_platform_init_locked(&argp->error);
> >>>> -               if (ret)
> >>>> -                       return ret;
> >>>> -       }
> >>>> +       bool shutdown_required = false;
> >>>> +       int ret, error;
> >>>>
> >>>>         if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> >>>>                 return -EFAULT;
> >>>> @@ -1952,6 +1960,16 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
> >>>>         data.cert_chain_len = input.cert_chain_len;
> >>>>
> >>>>  cmd:
> >>>> +       /* If platform is not in INIT state then transition it to INIT. */
> >>>> +       if (sev->state != SEV_STATE_INIT) {
> >>>> +               if (!writable)
> >>>> +                       goto e_free_cert;
> >>>> +               ret = __sev_platform_init_locked(&argp->error);
> >>>
> >>> Using argp->error for init instead of the ioctl-requested command
> >>> means that the user will have difficulty distinguishing which process
> >>> is at fault, no?
> >>>
> >>
> >> Not really, in case the SEV command has still not been issued, argp->error is still usable
> >> and returned back to the caller (no need to use a local error here), we are not overwriting
> >> the argp->error used for the original command/ioctl here.
> >>
> >
> > I mean in the case that argp->error is set to a value shared by the
> > command and init, it's hard to know what the problem was.
> > I'd like to ensure that the documentation is updated to reflect that
> > (in this case) if PDH_CERT_EXPORT returns INVALID_PLATFORM_STATE, then
> > it's because the platform was not in PSTATE.UNINIT state.
> > The new behavior of initializing when you need to now means that you
> > should have ruled out INVALID_PLATFORM_STATE as a possible value from
> > PDH_EXPORT_CERT. Same for SNP_CONFIG.
> >
> > There is not a 1-to-1 mapping between the ioctl commands and the SEV
> > commands now, so I think you need extra documentation to clarify the
> > new error space for at least pdh_export and set_config
> >
> > SNP_PLATFORM_STATUS, VLEK_LOAD, and SNP_COMMIT appear to not
> > necessarily have a provenance confusion after looking closer.
> >
> >
>
> I am more of less trying to match the current behavior of sev_ioctl_do_pek_import()
> or sev_ioctl_do_pdh_export().
>
> All this is implementation specific handling so we can't update SEV/SNP firmware
> API specs documentation for this new error space, this is not a firmware specific return code.
>

I was just talking about the uapi for the ioctls, not AMD reference
documentation.

> But to maintain 1-to-1 mapping between the ioctl commands and the SEV/SNP commands,
> i think it will be better to handle this INIT in the same way as SHUTDOWN, which
> is to use a local error for INIT and in case of implicit INIT failures, let the
> error logs from __sev_platform_init_locked() OR __sev_snp_init_locked() be printed
> and always return INVALID_PLATFORM_STATE as error back to the caller.
>
> Thanks,
> Ashish
>


-- 
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

Powered by blists - more mailing lists