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

Kirill A. Shutemov
2022-03-17T17:33:46Z

On Thu, Mar 17, 2022 at 01:48:54AM +0100, Thomas Gleixner wrote:
> On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
> Hmm?

Does the changed version below address your concerns?

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 has to be called early in #VE handling. A "nested" #VE which
* occurs before this will raise a #DF and is not recoverable.
*
* The call retrieves the #VE info from the TDX module, which also
* clears the "#VE valid" flag. This must be done before anything else
* because any #VE that occurs while the valid flag is set will lead to
* #DF.
*
* Note, the TDX module treats virtual NMIs as inhibited if the #VE
* valid flag is set. It means that NMI=>#VE will not result in a #DF.
*/
tdx_module_call(TDX_GET_VEINFO, 0, 0, 0, 0, &out);

/* Transfer the output parameters */
ve->exit_reason = out.rcx;
ve->exit_qual = out.rdx;
ve->gla = out.r8;
ve->gpa = out.r9;
ve->instr_len = lower_32_bits(out.r10);
ve->instr_info = upper_32_bits(out.r10);
}

> > +/*
> > + * 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?

[ Disclaimer: I have limited understanding of the entry code complexity
and may miss some crucial details. But I try my best. ]

Yes, it is the same comment, but it is based on code audit, not only on
testing.

I claim that kernel does not do anything that can possibly trigger #VE
where kernel cannot deal with it:

- on syscall entry code before kernel stack is set up (few instructions
in the beginning of entry_SYSCALL_64())

- in NMI entry code (asm_exc_nmi()) before NMI nesting is safe:
+ for NMI from user mode, before switched to thread stack
+ for NMI from kernel, up to end_repead_nmi

After that points #VE is safe.

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

I don't have such intentions.

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

It is detectable by audit. The critical windows very limited and located
in the highly scrutinized entry code. But, yes, I cannot guarantee that
this code will be perfect forever.

Consequences of #VE in these critical windows are mentioned in the
comment:

Any exception in this window leads to hard to debug issues and can
be exploited for privilege escalation.

I have hard time understanding what I has to change here. Do you want
details of audit to be documented? Make consequences of #VE at the wrong
point to be more prominent in the comment?

--
Kirill A. Shutemov