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(-)
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
On Fri, Oct 8, 2021 at 9:52 AM Brijesh Singh <[email protected]> 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
>
>
On Thu, Oct 7, 2021 at 4:30 PM Marc Orr <[email protected]> wrote:
>
> On Tue, Oct 5, 2021 at 12:52 PM Peter Gonda <[email protected]> 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 <[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(-)
> >
> > 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
> >
On 10/12/21 7:34 AM, Peter Gonda wrote:
> On Fri, Oct 8, 2021 at 9:52 AM Brijesh Singh <[email protected]> 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