Re: [PATCH v8 1/4] crypto: ccp - Name -1 return value as SEV_RET_NO_FW_CALL

[Dionna Amalie Glaze]

On Sat, Dec 3, 2022 at 11:37 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Sat, Dec 03, 2022 at 10:58:39AM -0800, Dionna Amalie Glaze wrote:
> > It doesn't always overwrite psp_ret, such as the initial error checking.
> > The value remains uninitialized for -ENODEV, -EBUSY, -EINVAL.
> > Thus *error in __sev_platform_init_locked can be set to uninitialized
> > memory if psp_ret is not first initialized.
>
> Lemme see if I understand it correctly: you wanna signal that all early
> return cases in __sev_do_cmd_locked() are such that no firmware was
> called?
>
> I.e., everything before the first iowrite into the command buffer?
>
> But then the commit message says:
>
> "The PSP can return a "firmware error" code of -1 in circumstances where
> the PSP is not actually called."
>
> which is confusing. How can the PSP return something if it wasn't called?
>
> Or you mean those cases above where it would fail on some of the checks
> before issuing a SEV command? I think you do...
>
> So I see Tom has ACKed this but I have to ask: is the SEV spec not going
> to use -1 ever?
>

I'll confirm with Tom, since he's changing the GHCB spec for the
throttling value.

> Also, if this behavior is going to be user-visible, where are we
> documenting it? Especially if nothing in the kernel is looking at
> that value but only assigning it to a retval which gets looked at by
> userspace. Especially then this should be documented.
>
> Dunno, maybe somewhere in Documentation/x86/amd-memory-encryption.rst or
> maybe Tom would have a better idea.
>

Agreed it should be in both the Linux documentation and the GHCB spec.

> > That error points to the kernel copy of the user's argument struct,
> > which the ioctl always copies back. In the case of those error codes
> > then, without SEV_RET_NO_FW_CALL, user space will get uninitialized
> > kernel memory.
>
> Right, but having a return value which means "firmware wasn't called"
> sounds weird. Why does userspace care?
>

Arguably it shouldn't ever get this value. We're just not very
selective when we copy back the kernel copy of the ioctl argument.
In all cases user space should treat the value as undefined, but still
we don't want to leak uninitialized kernel stack values.

Host driver: only on platform init, should just see the negative error
value and not try to interpret the fw_err in the argument.
Still the data is copied back and therefore should not be
uninitialized kernel memory.
Possible name: SEV_RET_UNDEFINED, or a return value -1 anyway with a
comment that the argument is undefined.

Guest driver: The host is issuing a guest request on behalf of the
guest using patch 4/4 of this series.
The guest is responsible for keeping the sequence number in sync with
the PSP, so we want to track if the ghcb_hv_call completed
successfully to know we should continue with the incremented IV.
Otherwise we run the risk of the sequence numbers getting out of sync
and we lock down the VMPCK.

The guest driver actually sets exitinfo2 to  an undocumented 0xff
initial value just in case.
=If the host doesn't write back a documented EXIT_INFO_2 value like
invalid_len or throttled, then the kernel will emit a log with the
initial value 0xff (or -1 after this patch).

I've changed it to -1 to name the same kind of error across host and
guest: the communication with the PSP didn't complete successfully, so
the "error" value is not from the PSP.
This value can also get returned to user space during a -ENOTTY result.
We can call this NO_FW_CALL or UNDEFINED. I have no real preference.

Whatever value we set initially, the VMM can overwrite exitinfo2
during the ghcb_hv_call.
I'd rather that the "undefined" values were the same across both,
because the guest is merely receiving a value from the host's PSP
driver (or should be).
It keeps the enum for return values a bit tidier and not concerned
with whether the value is viewed from the host or guest.

I can see an argument for not using the PSP header for its enum type
and instead defining and documenting and using the separate the 0xff
value elsewhere, but this seemed as good a place as any.


> I mean, you can just as well return any of the negative values -ENODEV,
> -EBUSY, -EINVAL too, depending on where you exit. Having three different
> retvals could tell you where exactly it failed, even.
>

That's true, those values are already being returned to user space as
the result of the ioctl.

> But the question remains: why does userspace needs to know that the
> failure happened and firmware wasn't called, as long as it is getting
> something negative to signal an error?
>

I hope the above discussion is clear that it's purely a defined
"undefined" because being pickier about what to copy_to_user during
exceptional circumstances in order to not overwrite the user's fw_err
value seems an unnecessary amount of code.

> Thx.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette



-- 
-Dionna Glaze, PhD (she/her)