Re: [PATCH 04/26] x86/traps: Add #VE support for TDX guest

[Borislav Petkov <bp@alien8.de>]

On Tue, Dec 14, 2021 at 06:02:42PM +0300, Kirill A. Shutemov wrote:
> Virtualization Exceptions (#VE) are delivered to TDX guests due to
> specific guest actions which may happen in either user space or the
> kernel:
> 
>  * Specific instructions (WBINVD, for example)
>  * Specific MSR accesses
>  * Specific CPUID leaf accesses
>  * Access to unmapped pages (EPT violation)
> 
> In the settings that Linux will run in, virtual exceptions are never
> generated on accesses to normal, TD-private memory that has been
> accepted.
> 
> The #VE handler implementation is simplified by the fact that entry
> paths do not trigger #VE and that the handler may not be interrupted.
> Specifically, the implementation assumes that the entry paths do not
> access TD-shared memory, MMIO regions, use #VE triggering MSRs,
> instructions, or CPUID leaves that might generate #VE. Interrupts,
> including NMIs, are blocked by the hardware starting with #VE delivery
> until TDGETVEINFO is called. All of this combined  eliminates the
> chance of a #VE during the syscall gap, or paranoid entry paths.
> 
> After TDGETVEINFO, #VE could happen in theory (e.g. through an NMI),
> but it is expected not to happen because TDX expects NMIs not to
> trigger #VEs. Another case where #VE could happen is if the #VE
> exception panics, but in this case, since the platform is already in
> a panic state, nested #VE is not a concern.
> 
> If a guest kernel action which would normally cause a #VE occurs in
> the interrupt-disabled region before TDGETVEINFO, a #DF (fault
> exception) is delivered to the guest which will result in an oops
> (and should eventually be a panic, as it is expected panic_on_oops is
> set to 1 for TDX guests).

So until here there are a lot of expectations and assumptions. What
happens if those are violated?

What happens if the NMI handler triggers a #VE after all? Or where is it
enforced that TDX guests should set panic_on_oops?

It all reads really weird, like the TDX guest is a big bird which simply
sticks its head in the sand in the face of danger...

...

> +/*
> + * Handle the user initiated #VE.
> + *
> + * For example, executing the CPUID instruction from the user

"... from userspace... " no "the"

> + * space is a valid case and hence the resulting #VE had to

s/had/has/

> + * be handled.
> + *
> + * For dis-allowed or invalid #VE just return failure.
> + *
> + * Return True on success and False on failure.

You lost me here - function returns false unconditionally. And that
bla about CPUID from user being a valid case doesn't really look like
one when I look at the code. Especially since ve_raise_fault() sends a
SIGSEGV for user #VEs.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette