Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220802185534.735338-1-jackyli@google.com> <20220802185534.735338-3-jackyli@google.com>
 <5f52ffe6-03a4-a5bf-9c57-2c3951922a35@amd.com>
In-Reply-To: <5f52ffe6-03a4-a5bf-9c57-2c3951922a35@amd.com>
From:   Jacky Li <jackyli@google.com>
Date:   Tue, 16 Aug 2022 12:30:39 -0700
Message-ID: <CAJxe5cu=gB3-urLEVShDH_-2vmB2Q=FDAtBdcTVn=oaxTAjykg@mail.gmail.com>
Subject: Re: [PATCH 2/2] crypto: ccp - Fail the PSP initialization when
 writing psp data file failed
To:     Tom Lendacky <thomas.lendacky@amd.com>
Cc:     Brijesh Singh <brijesh.singh@amd.com>,
        John Allen <john.allen@amd.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Marc Orr <marcorr@google.com>, Alper Gun <alpergun@google.com>,
        Peter Gonda <pgonda@google.com>, linux-crypto@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Aug 10, 2022 at 1:37 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 8/2/22 13:55, Jacky Li wrote:
> > Currently the OS continues the PSP initialization when there is a write
> > failure to the init_ex_file. Therefore, the userspace would be told that
> > SEV is properly INIT'd even though the psp data file is not updated.
> > This is problematic because later when asked for the SEV data, the OS
> > won't be able to provide it.
> >
> > Fixes: 3d725965f836 ("crypto: ccp - Add SEV_INIT_EX support")
> > Reported-by: Peter Gonda <pgonda@google.com>
> > Signed-off-by: Jacky Li <jackyli@google.com>
> > ---
> >   drivers/crypto/ccp/sev-dev.c | 23 +++++++++++++----------
> >   1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 5bb2ae250d38..fd6bb01eb198 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -233,7 +233,7 @@ static int sev_read_init_ex_file(void)
> >       return 0;
> >   }
> >
> > -static void sev_write_init_ex_file(void)
> > +static int sev_write_init_ex_file(void)
> >   {
> >       struct sev_device *sev = psp_master->sev_data;
> >       struct file *fp;
> > @@ -243,14 +243,15 @@ static void sev_write_init_ex_file(void)
> >       lockdep_assert_held(&sev_cmd_mutex);
> >
> >       if (!sev_init_ex_buffer)
> > -             return;
> > +             return 0;
> >
> >       fp = open_file_as_root(init_ex_path, O_CREAT | O_WRONLY, 0600);
> >       if (IS_ERR(fp)) {
> > +             int ret = PTR_ERR(fp);
>
> Please put a blank line after the variable declaration.
>

Will do in the v2. Thanks for the reminder!

> >               dev_err(sev->dev,
> >                       "SEV: could not open file for write, error %ld\n",
> > -                     PTR_ERR(fp));
> > -             return;
> > +                     ret);
>
> You'll need to fix the kernel test robot report here.
>
> Thanks,
> Tom
>

Will change %ld to %d in v2. Thanks!

Thanks,
Jacky


> > +             return ret;
> >       }
> >
> >       nwrite = kernel_write(fp, sev_init_ex_buffer, NV_LENGTH, &offset);
> > @@ -261,18 +262,20 @@ static void sev_write_init_ex_file(void)
> >               dev_err(sev->dev,
> >                       "SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
> >                       NV_LENGTH, nwrite);
> > -             return;
> > +             return -EIO;
> >       }
> >
> >       dev_dbg(sev->dev, "SEV: write successful to NV file\n");
> > +
> > +     return 0;
> >   }
> >
> > -static void sev_write_init_ex_file_if_required(int cmd_id)
> > +static int sev_write_init_ex_file_if_required(int cmd_id)
> >   {
> >       lockdep_assert_held(&sev_cmd_mutex);
> >
> >       if (!sev_init_ex_buffer)
> > -             return;
> > +             return 0;
> >
> >       /*
> >        * Only a few platform commands modify the SPI/NV area, but none of the
> > @@ -287,10 +290,10 @@ static void sev_write_init_ex_file_if_required(int cmd_id)
> >       case SEV_CMD_PEK_GEN:
> >               break;
> >       default:
> > -             return;
> > +             return 0;
> >       }
> >
> > -     sev_write_init_ex_file();
> > +     return sev_write_init_ex_file();
> >   }
> >
> >   static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> > @@ -363,7 +366,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >                       cmd, reg & PSP_CMDRESP_ERR_MASK);
> >               ret = -EIO;
> >       } else {
> > -             sev_write_init_ex_file_if_required(cmd);
> > +             ret = sev_write_init_ex_file_if_required(cmd);
> >       }
> >
> >       print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,