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

[Brijesh Singh]

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@xxxxxxxxxx>
>> Cc: "Radim KrÄmÃÅ" <rkrcmar@xxxxxxxxxx>
>> Cc: Borislav Petkov <bp@xxxxxxx>
>> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
>> Cc: Gary Hook <gary.hook@xxxxxxx>
>> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> Cc: linux-crypto@xxxxxxxxxxxxxxx
>> Cc: kvm@xxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
>> ---
>>  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;
>