Re: [PATCH] crypto: ccp - Consolidate sev INIT logic

[Marc Orr]

On Tue, Oct 5, 2021 at 12:52 PM Peter Gonda <pgonda@xxxxxxxxxx> wrote:
>
> Adds new helper function sev_init_if_required() for use in sev_ioctl().
> The function calls __sev_platform_init_locked() if the command requires
> the PSP's internal state be at least SEV_STATE_INIT. This consolidates
> many checks scattered through out the ioctl delegation functions.
>
> Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx>
> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Cc: Brijesh Singh <brijesh.singh@xxxxxxx>
> Cc: Marc Orr <marcorr@xxxxxxxxxx>
> Cc: Joerg Roedel <jroedel@xxxxxxx>
> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Cc: David Rientjes <rientjes@xxxxxxxxxx>
> Cc: John Allen <john.allen@xxxxxxx>
> Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> Cc: linux-crypto@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  drivers/crypto/ccp/sev-dev.c | 63 +++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e09925d86bf3..071d57fec4c4 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -386,24 +386,14 @@ 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;
> -
>         if (!writable)
>                 return -EPERM;

This check can be removed because it's is handled by
`sev_init_if_required()`. Same is true for all the other commands.

>
> -       if (sev->state == SEV_STATE_UNINIT) {
> -               rc = __sev_platform_init_locked(&argp->error);
> -               if (rc)
> -                       return rc;
> -       }
> -
>         return __sev_do_cmd_locked(cmd, NULL, &argp->error);
>  }
>
>  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;
>         struct sev_data_pek_csr data;
>         void __user *input_address;
> @@ -435,12 +425,6 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>         data.len = input.length;
>
>  cmd:
> -       if (sev->state == SEV_STATE_UNINIT) {
> -               ret = __sev_platform_init_locked(&argp->error);
> -               if (ret)
> -                       goto e_free_blob;
> -       }
> -
>         ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
>
>          /* If we query the CSR length, FW responded with expected data. */
> @@ -586,7 +570,6 @@ static int sev_update_firmware(struct device *dev)
>
>  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;
>         void *pek_blob, *oca_blob;
> @@ -617,17 +600,10 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
>         data.oca_cert_address = __psp_pa(oca_blob);
>         data.oca_cert_len = input.oca_cert_len;
>
> -       /* If platform is not in INIT state then transition it to INIT */
> -       if (sev->state != SEV_STATE_INIT) {
> -               ret = __sev_platform_init_locked(&argp->error);
> -               if (ret)
> -                       goto e_free_oca;
> -       }
> -
>         ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
>
> -e_free_oca:
>         kfree(oca_blob);
> +
>  e_free_pek:
>         kfree(pek_blob);
>         return ret;
> @@ -730,7 +706,6 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
>
>  static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>  {
> -       struct sev_device *sev = psp_master->sev_data;
>         struct sev_user_data_pdh_cert_export input;
>         void *pdh_blob = NULL, *cert_blob = NULL;
>         struct sev_data_pdh_cert_export data;
> @@ -738,16 +713,6 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>         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;
> -       }
> -
>         if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>                 return -EFAULT;
>
> @@ -819,6 +784,26 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>         return ret;
>  }
>
> +static int sev_init_if_required(int cmd_id, bool writable,
> +                               struct sev_issue_cmd *argp)
> +{
> +       struct sev_device *sev = psp_master->sev_data;
> +
> +       lockdep_assert_held(&sev_cmd_mutex);
> +
> +       if (!writable)
> +               return -EPERM;
> +
> +       if (cmd_id == SEV_FACTORY_RESET || cmd_id == SEV_PLATFORM_STATUS ||
> +           cmd_id == SEV_GET_ID || cmd_id == SEV_GET_ID2)
> +               return 0;

I really like this patch and would like to see it get reviewed and
merged. I've often thought of writing up a similar patch every time I
look at the PSP code, but never took the initiative to do it myself.
Overall, I wonder if it's trying too hard to reduce redundant code. In
particular, we could avoid this awkward check if we put this helper
inline, in the command helpers themselves. Perhaps we split this out
into two helpers or instead add a parameter to this helper to control
whether to check if `state` is `SEV_STATE_UNINIT`. What do you think?

> +
> +       if (sev->state == SEV_STATE_UNINIT)
> +               return __sev_platform_init_locked(&argp->error);
> +
> +       return 0;
> +}
> +
>  static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>  {
>         void __user *argp = (void __user *)arg;
> @@ -840,8 +825,11 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>
>         mutex_lock(&sev_cmd_mutex);
>
> -       switch (input.cmd) {
> +       ret = sev_init_if_required(input.cmd, writable, &input);
> +       if (ret)
> +               goto copy_out;
>
> +       switch (input.cmd) {

nit: Not sure what changed on this line. Was there an unintended
whitespace change here?

>         case SEV_FACTORY_RESET:
>                 ret = sev_ioctl_do_reset(&input, writable);
>                 break;
> @@ -875,6 +863,7 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>                 goto out;
>         }
>
> +copy_out:
>         if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
>                 ret = -EFAULT;
>  out:
> --
> 2.33.0.800.g4c38ced690-goog
>