Date:   Thu, 23 Dec 2021 20:45:40 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc:     tglx@linutronix.de, mingo@redhat.com, dave.hansen@intel.com,
        luto@kernel.org, peterz@infradead.org,
        sathyanarayanan.kuppuswamy@linux.intel.com, aarcange@redhat.com,
        ak@linux.intel.com, dan.j.williams@intel.com, david@redhat.com,
        hpa@zytor.com, jgross@suse.com, jmattson@google.com,
        joro@8bytes.org, jpoimboe@redhat.com, knsathya@kernel.org,
        pbonzini@redhat.com, sdeep@vmware.com, seanjc@google.com,
        tony.luck@intel.com, vkuznets@redhat.com, wanpengli@tencent.com,
        x86@kernel.org, linux-kernel@vger.kernel.org,
        Sean Christopherson <sean.j.christopherson@intel.com>
Subject: Re: [PATCH 04/26] x86/traps: Add #VE support for TDX guest
Message-ID: <YcTR5HnkHi7CjVyx@zn.tnic>
References: <20211214150304.62613-1-kirill.shutemov@linux.intel.com>
 <20211214150304.62613-5-kirill.shutemov@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211214150304.62613-5-kirill.shutemov@linux.intel.com>
Precedence: bulk

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