[PATCH] crypto: ccp - Consolidate sev INIT logic

[PATCH] crypto: ccp - Consolidate sev INIT logic

[Peter Gonda]

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@google.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Marc Orr <marcorr@google.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Rientjes <rientjes@google.com>
Cc: John Allen <john.allen@amd.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 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;
 
-	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;
+
+	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) {
 	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

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

[Marc Orr]

On Tue, Oct 5, 2021 at 12:52 PM Peter Gonda <pgonda@google.com> 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@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  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
>

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

[Brijesh Singh]

On 10/5/21 12:52 PM, Peter Gonda wrote:
>  
> +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;
> +
> +	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;

We need to call this function only for the SEV commands (i.e input.cmd
>=0 && input.cmd <= SEV_GET_ID2). Otherwise a invalid command may
trigger SEV_INIT. e.g below sequence:

1) SEV_FACTORY_RESET   // this will transition the fw to UNINIT state.

2) <INVALID_CMD_ID>   // since fw was in uninit this invalid command
will initialize the fw and then later switch will fail.

thanks

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

[Peter Gonda]

On Fri, Oct 8, 2021 at 9:52 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
>
> On 10/5/21 12:52 PM, Peter Gonda wrote:
> >
> > +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;
> > +
> > +     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;
>
> We need to call this function only for the SEV commands (i.e input.cmd
> >=0 && input.cmd <= SEV_GET_ID2). Otherwise a invalid command may
> trigger SEV_INIT. e.g below sequence:
>
> 1) SEV_FACTORY_RESET   // this will transition the fw to UNINIT state.
>
> 2) <INVALID_CMD_ID>   // since fw was in uninit this invalid command
> will initialize the fw and then later switch will fail.

Good catch, I took Marc's suggested approach for a V2. Does that sound
reasonable?

>
> thanks
>
>

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

[Peter Gonda]

On Thu, Oct 7, 2021 at 4:30 PM Marc Orr <marcorr@google.com> wrote:
>
> On Tue, Oct 5, 2021 at 12:52 PM Peter Gonda <pgonda@google.com> 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@google.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Cc: Marc Orr <marcorr@google.com>
> > Cc: Joerg Roedel <jroedel@suse.de>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: John Allen <john.allen@amd.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: linux-crypto@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  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.

This check is still needed on commands that "write" to the PSP I
think. Since the command SEV_CMD_PEK_GEN makes the PSP write a new PEK
to its storage I think this command still needs the file to be open
writeable. Same with 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?

That sounds good. I've moved calls to sev_init_if_required into the
command functions.

>
> > +
> > +       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?

Fixed for V2.

>
> >         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
> >

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

[Brijesh Singh]

On 10/12/21 7:34 AM, Peter Gonda wrote:
> On Fri, Oct 8, 2021 at 9:52 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
>>
>> On 10/5/21 12:52 PM, Peter Gonda wrote:
>>> +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;
>>> +
>>> +     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;
>> We need to call this function only for the SEV commands (i.e input.cmd
>>> =0 && input.cmd <= SEV_GET_ID2). Otherwise a invalid command may
>> trigger SEV_INIT. e.g below sequence:
>>
>> 1) SEV_FACTORY_RESET   // this will transition the fw to UNINIT state.
>>
>> 2) <INVALID_CMD_ID>   // since fw was in uninit this invalid command
>> will initialize the fw and then later switch will fail.
> Good catch, I took Marc's suggested approach for a V2. Does that sound
> reasonable?

Yes, that works.

thanks