Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211005195213.2905030-1-pgonda@google.com> <fdf0c263-38e9-7780-d0ac-943b6d2dd3a3@amd.com>
In-Reply-To: <fdf0c263-38e9-7780-d0ac-943b6d2dd3a3@amd.com>
From:   Peter Gonda <pgonda@google.com>
Date:   Tue, 12 Oct 2021 08:34:33 -0600
Message-ID: <CAMkAt6piNiJdHDMzSqgm-aQ0o-xxM=ax4qMgn81fKzVuaNukPg@mail.gmail.com>
Subject: Re: [PATCH] crypto: ccp - Consolidate sev INIT logic
To:     Brijesh Singh <brijesh.singh@amd.com>
Cc:     "Lendacky, Thomas" <thomas.lendacky@amd.com>,
        Marc Orr <marcorr@google.com>, Joerg Roedel <jroedel@suse.de>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        David Rientjes <rientjes@google.com>,
        John Allen <john.allen@amd.com>,
        "David S. Miller" <davem@davemloft.net>,
        linux-crypto@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 8, 2021 at 9:52 AM Brijesh Singh <brijesh.singh@amd.com> 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
>
>