Re: [PATCHv6 07/30] x86/traps: Add #VE support for TDX guest

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
> +void tdx_get_ve_info(struct ve_info *ve)
> +{
> +	struct tdx_module_output out;
> +
> +	/*
> +	 * Called during #VE handling to retrieve the #VE info from the
> +	 * TDX module.
> +	 *
> +	 * This should called done early in #VE handling.  A "nested"


... has to be called early ..

> +	 * #VE which occurs before this will raise a #DF and is not
> +	 * recoverable.
> +	 */
> +	tdx_module_call(TDX_GET_VEINFO, 0, 0, 0, 0, &out);
> +
> +	/* Interrupts and NMIs can be delivered again. */

Please put a new line between this comment and the code below because
they are completely unrelated. Also interrupts cannot be delivered here
because this code runs with interrupts disabled ...

And I rather have this comment above the tdx_module_call() invocation:

	 * recoverable.
         *
         * <Useful comment about VE info and NMIs>

When I reviewed this last time, Sean provided a very concise comment for
this.

         */
         tdx_module_call(...);
         
         /* Transfer the output parameters */

> +	ve->exit_reason = out.rcx;
        ....

Hmm?

> +/*
> + * 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 specific guest physical addresses
> + *
> + * In the settings that Linux will run in, virtualization exceptions are
> + * never generated on accesses to normal, TD-private memory that has been
> + * accepted.
> + *
> + * Syscall entry code has a critical window where the kernel stack is not
> + * yet set up. Any exception in this window leads to hard to debug issues
> + * and can be exploited for privilege escalation. Exceptions in the NMI
> + * entry code also cause issues. Returning from the exception handler with
> + * IRET will re-enable NMIs and nested NMI will corrupt the NMI stack.
> + *
> + * For these reasons, the kernel avoids #VEs during the syscall gap and
> + * the NMI entry code. Entry code paths do not access TD-shared memory,
> + * MMIO regions, use #VE triggering MSRs, instructions, or CPUID leaves
> + * that might generate #VE.

I asked that before:

  "How is that enforced or validated? What checks for a violation of that
   assumption?"

This is still exactly the same comment which is based on testing which
did not yet explode in your face, right?

So what's the point of this blurb? Create expectations which are not
accountable?

The point is that any #VE in such a code path is fatal and you better
come up with some reasonable explanation why this is not the case in
those code pathes and how a potential violation of that assumption might
be detected especially in rarely used corner cases. If such a violation
is not detectable by audit, CI, static code analysis or whatever then
document the consequences instead of pretending that the problem does
not exist and the kernel is perfect today and forever.

Thanks,

        tglx