Patchwork [Part2,v5.1,12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command

login
register
mail settings
Submitter Borislav Petkov
Date Oct. 12, 2017, 6:28 p.m.
Message ID <20171012182844.kjv5zab7o6fwwwdk@pd.tnic>
Download mbox | patch
Permalink /patch/10002483/
State Not Applicable
Delegated to: Herbert Xu
Headers show

Comments

Borislav Petkov - Oct. 12, 2017, 6:28 p.m.
On Fri, Oct 06, 2017 at 08:06:03PM -0500, Brijesh Singh wrote:
> The SEV_PEK_GEN command is used to generate a new Platform Endorsement
> Key (PEK). The command is defined in SEV spec section 5.6.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)

Just small fixups. Worth noting is this:

        if (do_shutdown)
-               sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+               ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);

I think we want to return the result of the *last* command
executed, which, in the do_shutdown=1 case is SEV_CMD_SHUTDOWN, not
SEV_CMD_PEK_GEN.

---
Brijesh Singh - Oct. 12, 2017, 8:11 p.m.
On 10/12/17 1:28 PM, Borislav Petkov wrote:
> On Fri, Oct 06, 2017 at 08:06:03PM -0500, Brijesh Singh wrote:
>> The SEV_PEK_GEN command is used to generate a new Platform Endorsement
>> Key (PEK). The command is defined in SEV spec section 5.6.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: Gary Hook <gary.hook@amd.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: linux-crypto@vger.kernel.org
>> Cc: kvm@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  drivers/crypto/ccp/psp-dev.c | 68 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 68 insertions(+)
> Just small fixups. Worth noting is this:
>
>         if (do_shutdown)
> -               sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> +               ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>
> I think we want to return the result of the *last* command
> executed, which, in the do_shutdown=1 case is SEV_CMD_SHUTDOWN, not
> SEV_CMD_PEK_GEN.

Lets  consider this scenario
1- platform is in uninit state, we transition it to INIT
2- PEK_GEN command failed
3- since we have transitioned the platform in INIT state hence we must
call the shutdown otherwise we will leave the system in wrong state. The
shutdown command will most probably succeed and we will look the ret value

> ---
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index d02f48e356e8..31045ea7e798 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -211,7 +211,7 @@ static int sev_platform_get_state(int *state, int *error)
>  	if (!data)
>  		return -ENOMEM;
>  
> -	ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
> +	ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
>  	if (!ret)
>  		*state = data->state;
>  
> @@ -228,7 +228,7 @@ static int sev_firmware_init(int *error)
>  	if (!data)
>  		return -ENOMEM;
>  
> -	rc = sev_handle_cmd(SEV_CMD_INIT, data, error);
> +	rc = sev_do_cmd(SEV_CMD_INIT, data, error);
>  
>  	kfree(data);
>  	return rc;
> @@ -240,8 +240,8 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
>  	int ret, state;
>  
>  	/*
> -	 * PEK_GEN command can be issued only when firmware is in INIT state.
> -	 * If firmware is in UNINIT state then we transition it in INIT state
> +	 * The PEK_GEN command can be issued only when the firmware is in INIT
> +	 * state. If it is in UNINIT state then we transition it in INIT state
>  	 * and issue the command.
>  	 */
>  	ret = sev_platform_get_state(&state, &argp->error);
> @@ -258,10 +258,10 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
>  		do_shutdown = 1;
>  	}
>  
> -	ret = sev_handle_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
> +	ret = sev_do_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
>  
>  	if (do_shutdown)
> -		sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> +		ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>  
>  	return ret;
>  }
> @@ -291,10 +291,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>  		ret = sev_ioctl_do_platform_status(&input);
>  		break;
>  
> -	case SEV_PEK_GEN: {
> +	case SEV_PEK_GEN:
>  		ret = sev_ioctl_pek_gen(&input);
>  		break;
> -	}
> +
>  	default:
>  		ret = -EINVAL;
>  		goto out;
>
Borislav Petkov - Oct. 12, 2017, 8:21 p.m.
On Thu, Oct 12, 2017 at 03:11:07PM -0500, Brijesh Singh wrote:
> Lets  consider this scenario
> 1- platform is in uninit state, we transition it to INIT
> 2- PEK_GEN command failed
> 3- since we have transitioned the platform in INIT state hence we must
> call the shutdown otherwise we will leave the system in wrong state. The
> shutdown command will most probably succeed and we will look the ret value

Sure but what do you do if the main command, i.e., PEK_GEN succeeds but
the shutdown command fails?

You probably should carve out the whole shutdown order in separate
functions. I mean, the sequences do repeat in a couple of functions so
you could do:

ioctl:

	case <CMD>:

		init_platform()
		do_main_cmd()
		shutdown_platform()
	break;

and this way you have everything nicely separated and retvals properly
tracked...

Hmmm?
Brijesh Singh - Oct. 12, 2017, 8:34 p.m.
On 10/12/17 3:21 PM, Borislav Petkov wrote:
> On Thu, Oct 12, 2017 at 03:11:07PM -0500, Brijesh Singh wrote:
>> Lets  consider this scenario
>> 1- platform is in uninit state, we transition it to INIT
>> 2- PEK_GEN command failed
>> 3- since we have transitioned the platform in INIT state hence we must
>> call the shutdown otherwise we will leave the system in wrong state. The
>> shutdown command will most probably succeed and we will look the ret value
> Sure but what do you do if the main command, i.e., PEK_GEN succeeds but
> the shutdown command fails?
>
> You probably should carve out the whole shutdown order in separate
> functions. I mean, the sequences do repeat in a couple of functions so
> you could do:
>
> ioctl:
>
> 	case <CMD>:
>
> 		init_platform()
> 		do_main_cmd()
> 		shutdown_platform()
> 	break;
>
> and this way you have everything nicely separated and retvals properly
> tracked...
>
> Hmmm?

Some commands are allowed in INIT and WORKING, some in UINIT only,  some
WORKING, and others in all the state. We need to follow the platform
state machine. I will see what I can do. thanks

Patch

diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d02f48e356e8..31045ea7e798 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -211,7 +211,7 @@  static int sev_platform_get_state(int *state, int *error)
 	if (!data)
 		return -ENOMEM;
 
-	ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+	ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
 	if (!ret)
 		*state = data->state;
 
@@ -228,7 +228,7 @@  static int sev_firmware_init(int *error)
 	if (!data)
 		return -ENOMEM;
 
-	rc = sev_handle_cmd(SEV_CMD_INIT, data, error);
+	rc = sev_do_cmd(SEV_CMD_INIT, data, error);
 
 	kfree(data);
 	return rc;
@@ -240,8 +240,8 @@  static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
 	int ret, state;
 
 	/*
-	 * PEK_GEN command can be issued only when firmware is in INIT state.
-	 * If firmware is in UNINIT state then we transition it in INIT state
+	 * The PEK_GEN command can be issued only when the firmware is in INIT
+	 * state. If it is in UNINIT state then we transition it in INIT state
 	 * and issue the command.
 	 */
 	ret = sev_platform_get_state(&state, &argp->error);
@@ -258,10 +258,10 @@  static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
 		do_shutdown = 1;
 	}
 
-	ret = sev_handle_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
+	ret = sev_do_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
 
 	if (do_shutdown)
-		sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+		ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
 
 	return ret;
 }
@@ -291,10 +291,10 @@  static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
 		ret = sev_ioctl_do_platform_status(&input);
 		break;
 
-	case SEV_PEK_GEN: {
+	case SEV_PEK_GEN:
 		ret = sev_ioctl_pek_gen(&input);
 		break;
-	}
+
 	default:
 		ret = -EINVAL;
 		goto out;