Re: [PATCH v4 14/22] x86/fpu/xstate: Expand the xstate buffer on the first use of dynamic user state

From: Andy Lutomirski <luto@amacapital.net>
To: Dave Hansen <dave.hansen@intel.com>
Cc: Len Brown <lenb@kernel.org>,
	"Liu, Jing2" <jing2.liu@linux.intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@suse.de>, Ingo Molnar <mingo@kernel.org>,
	X86 ML <x86@kernel.org>, Len Brown <len.brown@intel.com>,
	"Liu, Jing2" <jing2.liu@intel.com>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"Bae, Chang Seok" <chang.seok.bae@intel.com>
Subject: Re: [PATCH v4 14/22] x86/fpu/xstate: Expand the xstate buffer on the first use of dynamic user state
Date: Wed, 24 Mar 2021 15:12:43 -0700	[thread overview]
Message-ID: <95242657-E9AC-470C-9834-40BDA2E852B9@amacapital.net> (raw)
In-Reply-To: <90ea8d01-7fa5-100e-8590-d1ed80eb0d6a@intel.com>

> On Mar 24, 2021, at 2:58 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
> On 3/24/21 2:42 PM, Andy Lutomirski wrote:
>>>>> 3. user space always uses fully uncompacted XSAVE buffers.
>>>>> 
>>>> There is no reason we have to do this for new states. Arguably we
>>>> shouldn’t for AMX to avoid yet another altstack explosion.
>>> The thing that's worried me is that the list of OS-enabled states is
>>> visible to apps via XGETBV.  It doesn't seem too much of a stretch to
>>> think that apps will see AMX enabled with XGETBV and them assume that
>>> it's on the signal stack.
>>> 
>>> Please tell me I'm being too paranoid.  If we can break this
>>> assumption, it would get rid of a lot of future pain.
>> There are no AMX apps. I sure hope that there are no apps that
>> enumerate xfeatures with CPUID and try to decode the mess in the
>> signal stack.
> 
> I don't think they quite need to decode it in order to be screwed over a
> bit.  For instance, I don't think it's too crazy if someone did:
> 
>    xcr0 = xgetbv(0);
>    xrstor(xcr0, &sig_stack[something]);
>    // change some registers
>    xsave(xcr0, &sig_stack[something]);
> 
> The XRSTOR would work fine, but the XSAVE would overflow the stack
> because it would save the AMX state.  It also *looks* awfully benign.
> This is true even if the silly signal handler didn't know about AMX at
> *ALL*.
> 
> A good app would have checked that the xfeatures field in the header
> matched xcr0.

Ugh.

On the other hand, if we require a syscall to flip the AMX bit in XCR0, we could maybe get away with saying that programs that flip the bit and don’t understand the new ABI get to keep both pieces.

I don’t live futzing with the ABI like this, but AMX is really only barely compatible with everything before it.