Re: [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command
On Thu, Oct 12, 2017 at 03:21:04PM -0500, Brijesh Singh wrote: > We need to follow the platform state machine logic defined in SEV spec > section 5.1.2. The PEK_GEN can not be issued when platform is in WORKING > state because the command actually re-generate the identity of the > platform itself (in other words re-generate the Platform Endorsement > Key). Whereas, the PDH_GEN command is used for re-generating Platform > Diffie-Hellman Key which can be changed while the guest is running. I see. So the proposition to carve out and split the platform *init commands might come in handy here too... -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --
Re: [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command
On 10/12/17 1:48 PM, Borislav Petkov wrote: ... > On Fri, Oct 06, 2017 at 08:06:04PM -0500, Brijesh Singh wrote: >> The SEV_PDH_GEN command is used to re-generate the Platform >> Diffie-Hellman (PDH) key. The command is defined in SEV spec section >> 5.9. >> >> Cc: Paolo Bonzini>> Cc: "Radim Krčmář" >> Cc: Borislav Petkov >> Cc: Herbert Xu >> Cc: Gary Hook >> Cc: Tom Lendacky >> Cc: linux-crypto@vger.kernel.org >> Cc: k...@vger.kernel.org >> Cc: linux-ker...@vger.kernel.org >> Signed-off-by: Brijesh Singh >> --- >> drivers/crypto/ccp/psp-dev.c | 32 >> 1 file changed, 32 insertions(+) >> >> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c >> index 03d7bd03ad58..28efb7a9245a 100644 >> --- a/drivers/crypto/ccp/psp-dev.c >> +++ b/drivers/crypto/ccp/psp-dev.c >> @@ -271,6 +271,34 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp) >> return ret; >> } >> >> +static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp) >> +{ >> +int ret, state, do_shutdown = 0; >> + >> +/* >> + * PDH_GEN command can be issued when platform is in INIT or WORKING >> + * state. If we are in UNINIT state then transition in INIT state >> + * before issuing the command. >> + */ >> +ret = sev_platform_get_state(, >error); >> +if (ret) >> +return ret; >> + > Why isn't this function doing: > > if (state == SEV_STATE_WORKING) { > return -EBUSY; > > like the PEK_GEN one? We need to follow the platform state machine logic defined in SEV spec section 5.1.2. The PEK_GEN can not be issued when platform is in WORKING state because the command actually re-generate the identity of the platform itself (in other words re-generate the Platform Endorsement Key). Whereas, the PDH_GEN command is used for re-generating Platform Diffie-Hellman Key which can be changed while the guest is running. > Because if so, you can convert it and the PEK_GEN one into a single > function doing the work and wrappers handing in the command to avoid the > code duplication. > >> +if (state == SEV_STATE_UNINIT) { >> +ret = sev_firmware_init(>error); >> +if (ret) >> +return ret; >> +do_shutdown = 1; >> +} >> + >> +ret = sev_handle_cmd(SEV_CMD_PDH_GEN, 0, >error); >> + >> +if (do_shutdown) >> +sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL); >> + >> +return ret; >> +} >> + >> static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long >> arg) >> { >> void __user *argp = (void __user *)arg; >> @@ -300,6 +328,10 @@ static long sev_ioctl(struct file *file, unsigned int >> ioctl, unsigned long arg) >> ret = sev_ioctl_pek_gen(); >> break; >> } >> +case SEV_PDH_GEN: { >> +ret = sev_ioctl_pdh_gen(); >> +break; >> +} > And those curly braces can go, as before. >
Re: [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command
On Fri, Oct 06, 2017 at 08:06:04PM -0500, Brijesh Singh wrote: > The SEV_PDH_GEN command is used to re-generate the Platform > Diffie-Hellman (PDH) key. The command is defined in SEV spec section > 5.9. > > Cc: Paolo Bonzini> Cc: "Radim Krčmář" > Cc: Borislav Petkov > Cc: Herbert Xu > Cc: Gary Hook > Cc: Tom Lendacky > Cc: linux-crypto@vger.kernel.org > Cc: k...@vger.kernel.org > Cc: linux-ker...@vger.kernel.org > Signed-off-by: Brijesh Singh > --- > drivers/crypto/ccp/psp-dev.c | 32 > 1 file changed, 32 insertions(+) > > diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c > index 03d7bd03ad58..28efb7a9245a 100644 > --- a/drivers/crypto/ccp/psp-dev.c > +++ b/drivers/crypto/ccp/psp-dev.c > @@ -271,6 +271,34 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp) > return ret; > } > > +static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp) > +{ > + int ret, state, do_shutdown = 0; > + > + /* > + * PDH_GEN command can be issued when platform is in INIT or WORKING > + * state. If we are in UNINIT state then transition in INIT state > + * before issuing the command. > + */ > + ret = sev_platform_get_state(, >error); > + if (ret) > + return ret; > + Why isn't this function doing: if (state == SEV_STATE_WORKING) { return -EBUSY; like the PEK_GEN one? Because if so, you can convert it and the PEK_GEN one into a single function doing the work and wrappers handing in the command to avoid the code duplication. > + if (state == SEV_STATE_UNINIT) { > + ret = sev_firmware_init(>error); > + if (ret) > + return ret; > + do_shutdown = 1; > + } > + > + ret = sev_handle_cmd(SEV_CMD_PDH_GEN, 0, >error); > + > + if (do_shutdown) > + sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL); > + > + return ret; > +} > + > static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long > arg) > { > void __user *argp = (void __user *)arg; > @@ -300,6 +328,10 @@ static long sev_ioctl(struct file *file, unsigned int > ioctl, unsigned long arg) > ret = sev_ioctl_pek_gen(); > break; > } > + case SEV_PDH_GEN: { > + ret = sev_ioctl_pdh_gen(); > + break; > + } And those curly braces can go, as before. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --