Re: [PATCH] x86/cpufeatures: Force disable X86_FEATURE_ENQCMD and remove update_pasid()

From: Borislav Petkov <bp@alien8.de>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
	iommu@lists.linux-foundation.org, Ingo Molnar <mingo@redhat.com>,
	H Peter Anvin <hpa@zytor.com>, Andy Lutomirski <luto@kernel.org>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Christoph Hellwig <hch@infradead.org>,
	Peter Zijlstra <peterz@infradead.org>,
	David Woodhouse <dwmw2@infradead.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Tony Luck <tony.luck@intel.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Ashok Raj <ashok.raj@intel.com>,
	Jacob Jun Pan <jacob.jun.pan@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Sohil Mehta <sohil.mehta@intel.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>
Subject: Re: [PATCH] x86/cpufeatures: Force disable X86_FEATURE_ENQCMD and remove update_pasid()
Date: Mon, 31 May 2021 10:43:04 +0200	[thread overview]
Message-ID: <YLShmFEzddfm7WQs@zn.tnic> (raw)
In-Reply-To: <87mtsd6gr9.ffs@nanos.tec.linutronix.de>

On Sat, May 29, 2021 at 11:17:30AM +0200, Thomas Gleixner wrote:
> While digesting the XSAVE related horrors, which got introduced with the
> supervisor/user split, the recent addition of ENQCMD related functionality
> got on the radar and turned out to be similarly broken.
> 
> update_pasid(), which is only required when X86_FEATURE_ENQCMD is
> available, is invoked from two places:
> 
>  1) From switch_to() for the incoming task
> 
>  2) Via a SMP function call from the IOMMU/SMV code
> 
> #1 is half-ways correct as it hacks around the brokenness of get_xsave_addr()
>    by enforcing the state to be 'present', but all the conditionals in that
>    code are completely pointless for that.
> 
>    Also the invocation is just useless overhead because at that point
>    it's guaranteed that TIF_NEED_FPU_LOAD is set on the incoming task
>    and all of this can be handled at return to user space.
> 
> #2 is broken beyond repair. The comment in the code claims that it is safe
>    to invoke this in an IPI, but that's just wishful thinking.
> 
>    FPU state of a running task is protected by fregs_lock() which is
>    nothing else than a local_bh_disable(). As BH disabled regions run
>    usually with interrupts enabled the IPI can hit a code section which
>    modifies FPU state and there is absolutely no guarantee that any of the
>    assumptions which are made for the IPI case is true.

... so on a PASID system, your trivial reproducer would theoretically
fire the same way and corrupt FPU state just as well.

Hohumm, I'd say we need all those reproducers turned into proper self
tests and run on everything new that touches xstate. *At* *least*.

-- 
Regards/Gruss,
    Boris.

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