crypto: ccp - Consolidate sev INIT logic

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 <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc Brijesh Singh <[email protected]>

Cc Marc Orr <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Herbert Xu <[email protected]>

Cc David Rientjes <[email protected]>

Cc John Allen <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc [email protected]

---
 drivers/crypto/ccp/sev-dev.c | 63 +++++++++++++++---------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

drivers/crypto/ccp/sev-dev.c +0 -0

index e09925d86bf3..071d57fec4c4 100644

drivers/crypto/ccp/sev-dev.c +4 -33

-       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)
+{

drivers/crypto/ccp/sev-dev.c +7 -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;

drivers/crypto/ccp/sev-dev.c +1 -1

        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

Quoted 27 lines · 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 <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc Brijesh Singh <[email protected]>

Cc Marc Orr <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Herbert Xu <[email protected]>

Cc David Rientjes <[email protected]>

Cc John Allen <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc [email protected]

---
 drivers/crypto/ccp/sev-dev.c | 63 +++++++++++++++---------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

drivers/crypto/ccp/sev-dev.c +0 -0

index e09925d86bf3..071d57fec4c4 100644
@@ -386,24 +386,14 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 

drivers/crypto/ccp/sev-dev.c static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp) · line 390

Quoted 7 lines

drivers/crypto/ccp/sev-dev.c +0 -3

 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.

Quoted 89 lines

drivers/crypto/ccp/sev-dev.c +4 -33

-       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)
+{

drivers/crypto/ccp/sev-dev.c static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable) · line 799

Quoted 10 lines

drivers/crypto/ccp/sev-dev.c +10 -0

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

Quoted 20 lines

drivers/crypto/ccp/sev-dev.c +10 -1

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

drivers/crypto/ccp/sev-dev.c static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) · line 832

Quoted 1 line

drivers/crypto/ccp/sev-dev.c +1 -0

+       switch (input.cmd) {

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

Quoted 15 lines

drivers/crypto/ccp/sev-dev.c +1 -1

        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

drivers/crypto/ccp/sev-dev.c +20 -0

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

=0 && input.cmd <= SEV_GET_ID2). Otherwise a invalid command may

Quoted 25 lines · Peter Gonda

drivers/crypto/ccp/sev-dev.c +20 -0

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

drivers/crypto/ccp/sev-dev.c static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) · line 830

Quoted 7 lines

drivers/crypto/ccp/sev-dev.c +3 -1

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

Quoted 1 line

=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

Quoted 35 lines · Brijesh Singh

Quoted 24 lines · Peter Gonda

drivers/crypto/ccp/sev-dev.c +20 -0

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

drivers/crypto/ccp/sev-dev.c static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) · line 830

Quoted 7 lines

drivers/crypto/ccp/sev-dev.c +3 -1

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

Quoted 1 line

=0 && input.cmd <= SEV_GET_ID2). Otherwise a invalid command may

Quoted 6 lines

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?