Re: [PATCH V3 4/4] crypto: ccp - Add SEV_INIT_EX support

On Fri, Nov 12, 2021 at 4:39 PM Brijesh Singh [off-list ref] wrote:

On 11/12/21 10:55 AM, Peter Gonda wrote:

quoted

On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda [off-list ref] wrote:

quoted

On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh [off-list ref] wrote:

quoted

On 11/9/21 2:46 PM, Peter Gonda wrote:

quoted

On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson [off-list ref] wrote:

quoted

On Tue, Nov 09, 2021, Peter Gonda wrote:

quoted

On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson [off-list ref] wrote:

quoted

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.

quoted

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.

OK I'll leave as is.

quoted

quoted

quoted

quoted

+     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.

quoted

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?

quoted

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.

If this driver is builtin the filesystem will be unavailable during
__init. Using the existing retries already built into
sev_platform_init() also the file to be read once userspace is
running, meaning the file system is usable. As I tried to explain in
the commit message. We could remove the sev_platform_init call during
sev_pci_init since this only actually needs to be initialized when the
first command requiring it is issues (either reading some keys/certs
from the PSP or launching an SEV guest). Then userspace in both the
builtin and module usage would know running one of those commands
cause the file to be read for PSP usage. Tom any thoughts on this?

One thing to note is that if we do the INIT on the first command then
the first guest launch will take a longer. The init command is not
cheap (especially with the SNP, it may take a longer because it has to
do all those RMP setup etc). IIRC, in my early SEV series in I was doing
the INIT during the first command execution and based on the
recommendation moved to do the init on probe.

Should we add a module param to control whether to do INIT on probe or
delay until the first command ?

Thats a good point Brijesh. I've only been testing this with SEV and
ES so haven't noticed that long setup time. I like the idea of a
module parameter to decide when to INIT, that should satisfy Sean's
concern that the user doesn't know when the INIT_EX file would be read
and that there is extra retry code (duplicated between sev_pci_init
and all the PSP commands). I'll get started on that.

I need a little guidance on how to proceed with this. Should I have
the new module parameter 'psp_init_on_probe' just disable PSP init on
module init if false. Or should it also disable PSP init during
command flow if it's true?

I was thinking I should just have 'psp_init_on_probe' default to true,
and if false it stops the PSP init during sev_pci_init(). If I add the
second change that seems like it changes the ABI. Thoughts?

Good point that a module params may break the ABI. How about if we add a
new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
implementation will be similar to the PLATFORM_RESET; it will shutdown
the firmware then call INIT_EX. A platform provisioning tool may use ioctl.

Would just a 'skip_psp_init_on_probe' parameter be simpler. We default
to false but if users set it, we can skip that init attempt in
sev_pci_init(). The init attempts on all other commands that require
the INIT state would then provide users with INIT_EX functionality.
They would also know exactly when INIT or INIT_EX would be attempted
based on the parameter.

Otherwise a new ioctl sounds reasonable.

-Brijesh