On Thu, Feb 20, 2025 at 2:18 PM Kalra, Ashish <ashish.kalra@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx> wrote:
> >>>>
> >>>> From: Ashish Kalra <ashish.kalra@xxxxxxx>
> >>>>
> >>>> 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@xxxxxxx>
> >>>> ---
> >>>> 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)
