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

[Tom Lendacky]

On 12/5/22 11:05, Dionna Amalie Glaze wrote:
> 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.

The SEV API error codes are 16-bits in size, so you'll never see a -1.

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

Linux documentation, yes, GHCB spec, no.

Thanks,
Tom

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