linux-kernel - Re: [PATCH v2 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Mar 2021 10:14:02 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>
Cc:     Andi Kleen <ak@...ux.intel.com>,
        Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
        Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Raj Ashok <ashok.raj@...el.com>,
        Sean Christopherson <seanjc@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] x86/tdx: Handle MWAIT, MONITOR and WBINVD

On 3/27/21 3:54 PM, Kuppuswamy Sathyanarayanan wrote:
> +	/*
> +	 * Per Guest-Host-Communication Interface (GHCI) for Intel Trust
> +	 * Domain Extensions (Intel TDX) specification, sec 2.4,
> +	 * some instructions that unconditionally cause #VE (such as WBINVD,
> +	 * MONITOR, MWAIT) do not have corresponding TDCALL
> +	 * [TDG.VP.VMCALL <Instruction>] leaves, since the TD has been designed
> +	 * with no deterministic way to confirm the result of those operations
> +	 * performed by the host VMM.  In those cases, the goal is for the TD
> +	 * #VE handler to increment the RIP appropriately based on the VE
> +	 * information provided via TDCALL.
> +	 */

That's an awfully big comment.  Could you pare it down, please?  Maybe
focus on the fact that we should never get here and why, rather than
talking about some silly spec?

> +	case EXIT_REASON_WBINVD:
> +		pr_warn_once("WBINVD #VE Exception\n");

I actually think WBINVD in here should oops.  We use it for some really
important things.  If it can't be executed, and we're depending on it,
the kernel is in deep, deep trouble.

I think a noop here is dangerous.

> +	case EXIT_REASON_MONITOR_INSTRUCTION:
> +		/* Handle as nops. */
> +		break;

MONITOR is a privileged instruction, right?  So we can only end up in
here if the kernel screws up and isn't reading CPUID correctly, right?

That dosen't seem to me like something we want to suppress.  This needs
a warning, at least.  I assume that having a MONITOR instruction
immediately return doesn't do any harm.

> +	case EXIT_REASON_MWAIT_INSTRUCTION:
> +		/* MWAIT is supressed, not supposed to reach here. */
> +		WARN(1, "MWAIT unexpected #VE Exception\n");
> +		return -EFAULT;

How is MWAIT "supppressed"?