Date:   Tue, 9 Nov 2021 20:26:05 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Peter Gonda <pgonda@google.com>
Cc:     Thomas.Lendacky@amd.com, David Rientjes <rientjes@google.com>,
        Marc Orr <marcorr@google.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Joerg Roedel <jroedel@suse.de>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        John Allen <john.allen@amd.com>,
        "David S. Miller" <davem@davemloft.net>,
        Paolo Bonzini <pbonzini@redhat.com>,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support
Message-ID: <YYrZXRTukz3RccPN@google.com>
References: <20211102142331.3753798-1-pgonda@google.com>
 <20211102142331.3753798-5-pgonda@google.com>
 <YYquDWbkIwCkixxD@google.com>
 <CAMkAt6rHdsdD-L4PbZL7qaOY7GRHmApVJam0V0yY2BnYdhmPjA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAMkAt6rHdsdD-L4PbZL7qaOY7GRHmApVJam0V0yY2BnYdhmPjA@mail.gmail.com>
Precedence: bulk

On Tue, Nov 09, 2021, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <seanjc@google.com> wrote:
> > There's no need for this to be a function pointer, and the duplicate code can be
> > consolidated.
> >
> > static int sev_do_init_locked(int cmd, void *data, int *error)
> > {
> >         if (sev_es_tmr) {
> >                 /*
> >                  * Do not include the encryption mask on the physical
> >                  * address of the TMR (firmware should clear it anyway).
> >                  */
> >                 data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >                 data.tmr_address = __pa(sev_es_tmr);
> >                 data.tmr_len = SEV_ES_TMR_SIZE;
> >         }
> >         return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > }
> >
> > static int __sev_init_locked(int *error)
> > {
> >         struct sev_data_init data;
> >
> >         memset(&data, 0, sizeof(data));
> >         return sev_do_init_locked(cmd, &data, error);
> > }
> >
> > static int __sev_init_ex_locked(int *error)
> > {
> >         struct sev_data_init_ex data;
> >
> >         memset(&data, 0, sizeof(data));
> >         data.length = sizeof(data);
> >         data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >         data.nv_len = NV_LENGTH;
> >         return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > }
> 
> I am missing how this removes the duplication of the retry code,
> parameter checking, and other error checking code.. With what you have
> typed out I would assume I still need to function pointer between
> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> here?

Hmm.  Ah, I got distracted between the original thought, the realization that
the two commands used different structs, and typing up the above.

> Also is there some reason the function pointer is not acceptable?

It's not unacceptable, it would just be nice to avoid, assuming the alternative
is cleaner.  But I don't think any alternative is cleaner, since as you pointed
out the above is a half-baked thought.

> > > +     rc = init_function(error);
> > >       if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > >               /*
> > >                * INIT command returned an integrity check failure
> > > @@ -286,8 +423,8 @@ static int __sev_platform_init_locked(int *error)
> > >                * failed and persistent state has been erased.
> > >                * Retrying INIT command here should succeed.
> > >                */
> > > -             dev_dbg(sev->dev, "SEV: retrying INIT command");
> > > -             rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > +             dev_notice(sev->dev, "SEV: retrying INIT command");
> > > +             rc = init_function(error);
> >
> > The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > only writes back to the file if a relevant command was successful, which means
> > that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > with the same garbage data.
> 
> Ack my mistake, that comment is stale. I will update it so its correct
> for the INIT and INIT_EX flows.
> >
> > IMO, the behavior should be to read the file on load and then use the kernel buffer
> > without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > the kernel's internal state isn't blasted away.
> 
> One issue here is that the file read can fail on load so we use the
> late retry to guarantee we can read the file.

But why continue loading if reading the file fails on load?

> The other point seems like preference. Users may wish to shutdown the PSP FW,
> load a new file, and INIT_EX again with that new data. Why should we preclude
> them from that functionality?

I don't think we should preclude that functionality, but it needs to be explicitly
tied to a userspace action, e.g. either on module load or on writing the param to
change the path.  If the latter is allowed, then it needs to be denied if the PSP
is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
userspace will have a heck of a time even understanding what state has been used
to initialize the PSP.