Re: [PATCH v8 1/4] crypto: ccp - Name -1 return value as SEV_RET_NO_FW_CALL

[Dionna Amalie Glaze]

On Sat, Dec 3, 2022 at 4:26 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Fri, Nov 04, 2022 at 11:00:37PM +0000, Dionna Glaze wrote:
> > From: Peter Gonda <pgonda@xxxxxxxxxx>
> >
> > The PSP can return a "firmware error" code of -1 in circumstances where
> > the PSP is not actually called. To make this protocol unambiguous, we
>
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
>
> > add a constant naming the return value.
> >
> > Cc: Thomas Lendacky <Thomas.Lendacky@xxxxxxx>
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Cc: Joerg Roedel <jroedel@xxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Andy Lutomirsky <luto@xxxxxxxxxx>
> > Cc: John Allen <john.allen@xxxxxxx>
> > Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> > Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> >
> > Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx>
> > Signed-off-by: Dionna Glaze <dionnaglaze@xxxxxxxxxx>
> > ---
> >  drivers/crypto/ccp/sev-dev.c | 2 +-
> >  include/uapi/linux/psp-sev.h | 7 +++++++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 06fc7156c04f..97eb3544ab36 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -444,7 +444,7 @@ static int __sev_platform_init_locked(int *error)
> >  {
> >       struct psp_device *psp = psp_master;
> >       struct sev_device *sev;
> > -     int rc = 0, psp_ret = -1;
> > +     int rc = 0, psp_ret = SEV_RET_NO_FW_CALL;
> >       int (*init_function)(int *error);
> >
> >       if (!psp || !psp->sev_data)
>
> Ok, lemme chase down this flow here:
>
> __sev_platform_init_locked() calls that automatic variable function
> pointer ->init_function which already looks funky. See the end of this
> mail for a diff removing it and making the code more readable.
>

I'm fine removing it if possible for the sev-dev.c code, but I'll
still need the enum for the next patches in this series. I added it
specifically because of the uninitialized memory problem with `err`
that I witnessed in user space, and to replace the arbitrary 0xff
value in existing code.

> The called function can be one of two and both get the pointer to
> psp_ret as its only argument.
>
> 1. __sev_init_ex_locked() calls __sev_do_cmd_locked() and passes down
> *psp_ret.
>
> or
>
> 2. __sev_init_locked(). Ditto.
>
> Now, __sev_do_cmd_locked() will overwrite psp_ret when
> sev_wait_cmd_ioc() fails. So far so good.

It doesn't always overwrite psp_ret, such as the initial error checking.
The value remains uninitialized for -ENODEV, -EBUSY, -EINVAL.
Thus *error in __sev_platform_init_locked can be set to uninitialized
memory if psp_ret is not first initialized.
That error points to the kernel copy of the user's argument struct,
which the ioctl always copies back.
In the case of those error codes then, without SEV_RET_NO_FW_CALL,
user space will get uninitialized kernel memory.

>
> In the case __sev_do_cmd_locked() succeeds, it'll put there something
> else:
>
>         if (psp_ret)
>                 *psp_ret = reg & PSP_CMDRESP_ERR_MASK;
>
> So no caller will ever see SEV_RET_NO_FW_CALL, as far as I can tell.
>
> And looking further through the rest of the set, nothing tests
> SEV_RET_NO_FW_CALL - it only gets assigned.
>
> So why are we even bothering with this?
>
> You can hand in *psp_ret uninitialized and you'll get a value in all
> cases. Unless I'm missing an angle.
>

I think my above comment points out the wrinkle.

> > diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> > index 91b4c63d5cbf..1ad7f0a7e328 100644
> > --- a/include/uapi/linux/psp-sev.h
> > +++ b/include/uapi/linux/psp-sev.h
> > @@ -36,6 +36,13 @@ enum {
> >   * SEV Firmware status code
> >   */
> >  typedef enum {
> > +     /*
> > +      * This error code is not in the SEV spec but is added to convey that
> > +      * there was an error that prevented the SEV Firmware from being called.
> > +      * This is (u32)-1 since the firmware error code is represented as a
> > +      * 32-bit integer.
> > +      */
> > +     SEV_RET_NO_FW_CALL = 0xffffffff,
>
> What's wrong with having -1 here?
>

C++ brain not trusting what type enum has even in C. I can change it to -1.

> >       SEV_RET_SUCCESS = 0,
> >       SEV_RET_INVALID_PLATFORM_STATE,
> >       SEV_RET_INVALID_GUEST_STATE,
> > --
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 97eb3544ab36..8bc4209b338b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -440,12 +440,20 @@ static int __sev_init_ex_locked(int *error)
>         return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
>  }
>
> +static inline int __sev_do_init_locked(int *psp_ret)
> +{
> +       if (sev_init_ex_buffer)
> +               return __sev_init_ex_locked(psp_ret);
> +       else
> +
> +               return __sev_init_locked(psp_ret);
> +}
> +
>  static int __sev_platform_init_locked(int *error)
>  {
>         struct psp_device *psp = psp_master;
>         struct sev_device *sev;
> -       int rc = 0, psp_ret = SEV_RET_NO_FW_CALL;
> -       int (*init_function)(int *error);
> +       int rc = 0, psp_ret;
>
>         if (!psp || !psp->sev_data)
>                 return -ENODEV;
> @@ -456,15 +464,12 @@ static int __sev_platform_init_locked(int *error)
>                 return 0;
>
>         if (sev_init_ex_buffer) {
> -               init_function = __sev_init_ex_locked;
>                 rc = sev_read_init_ex_file();
>                 if (rc)
>                         return rc;
> -       } else {
> -               init_function = __sev_init_locked;
>         }
>
> -       rc = init_function(&psp_ret);
> +       rc = __sev_do_init_locked(&psp_ret);
>         if (rc && psp_ret == SEV_RET_SECURE_DATA_INVALID) {
>                 /*
>                  * Initialization command returned an integrity check failure
> @@ -473,9 +478,12 @@ static int __sev_platform_init_locked(int *error)
>                  * initialization function should succeed by replacing the state
>                  * with a reset state.
>                  */
> -               dev_err(sev->dev, "SEV: retrying INIT command because of SECURE_DATA_INVALID error. Retrying once to reset PSP SEV state.");
> -               rc = init_function(&psp_ret);
> +               dev_err(sev->dev,
> +"SEV: retrying INIT command because of SECURE_DATA_INVALID error. Retrying once to reset PSP SEV state.");
> +
> +               rc = __sev_do_init_locked(&psp_ret);
>         }
> +
>         if (error)
>                 *error = psp_ret;
>
> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> index 1ad7f0a7e328..a9ed9e846cd2 100644
> --- a/include/uapi/linux/psp-sev.h
> +++ b/include/uapi/linux/psp-sev.h
> @@ -42,7 +42,7 @@ typedef enum {
>          * This is (u32)-1 since the firmware error code is represented as a
>          * 32-bit integer.
>          */
> -       SEV_RET_NO_FW_CALL = 0xffffffff,
> +       SEV_RET_NO_FW_CALL = -1,
>         SEV_RET_SUCCESS = 0,
>         SEV_RET_INVALID_PLATFORM_STATE,
>         SEV_RET_INVALID_GUEST_STATE,
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette



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