Re: [PATCH v9 12/26] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state

["Bae, Chang Seok" <chang.seok.bae@intel.com>]

[ Cut out the API comments and other obvious ones here. ]

On Aug 18, 2021, at 09:24, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Jul 30, 2021 at 07:59:43AM -0700, Chang S. Bae wrote:
> 
>> +/**
>> + * xfd_switch - Switches the MSR IA32_XFD context if needed.
>> + * @prev:	The previous task's struct fpu pointer
>> + * @next:	The next task's struct fpu pointer
>> + */
>> +static inline void xfd_switch(struct fpu *prev, struct fpu *next)
>> +{
>> +	u64 prev_xfd_mask, next_xfd_mask;
>> +
>> +	if (!static_cpu_has(X86_FEATURE_XFD) || !xfd_capable())
> 
> cpu_feature_enabled(). Use that everywhere in your patchset. But you
> know already...

Yes, I did on my local.

>> +		return;
>> +
>> +	prev_xfd_mask = prev->state_mask & xfd_capable();
>> +	next_xfd_mask = next->state_mask & xfd_capable();
> 
> This is just plain misleading:
> 
> You're *AND*ing a mask with xfd_capable?!?
> 
> Just use xfeatures_mask_user_dynamic directly instead, as already
> mentioned.

Okay.

>> +	if (unlikely(prev_xfd_mask != next_xfd_mask))
>> +		xfd_write(xfd_capable() ^ next_xfd_mask);
>> +}
> 
> Here too.
> 
> Also, I must be missing something. Let's play with some imaginary masks:
> 
> prev->state_mask = 110b
> next->state_mask = 111b
> dyn		 = 101b
> 
> ("dyn" is short for xfeatures_mask_user_dynamic)
> 
> prev_xfd_mask = 100b
> next_xfd_mask = 101b
> 
> if (unlikely(100b != 101b))
> 	xfd_write(101b ^ 101b) == xfd_write(0)
> 
> so next has bits 2 and 0 set but the xfd write zaps them so next won't
> get any more #NMs for those states.
> 
> Why?

Because the next has already fully expanded the buffer -- its state_mask
equals to feature_mask_user_dynamic.

No more XFD event is needed for the task.

>> 
>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>> index a58800973aed..dd66d528afd8 100644
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -1112,6 +1112,45 @@ DEFINE_IDTENTRY(exc_device_not_available)
>> {
>> 	unsigned long cr0 = read_cr0();
>> 
>> +	if (boot_cpu_has(X86_FEATURE_XFD)) {
> 
> This whole thing wants to be in a separate function. Even the
> indentation level is begging for it.

Ah, it was once in a separate function until V4. Since trimmed down quite a
bit in V5, it has grown from there.

Let me fix this.

>> +		u64 xfd_err;
>> +
>> +		rdmsrl_safe(MSR_IA32_XFD_ERR, &xfd_err);
>> +		wrmsrl_safe(MSR_IA32_XFD_ERR, 0);
>> +
>> +		if (xfd_err) {
>> +			u64 xfd_event = xfd_err & xfd_capable();
>> +
>> +			if (WARN_ON(!xfd_event)) {
>> +				/*
>> +				 * Unexpected event is raised. But update XFD state to
>> +				 * unblock the task.
>> +				 */
>> +				xfd_write(xfd_read() & ~xfd_err);
> 
> So AFAIU, xfd_err points to some other feature which caused this
> exception.
> 
> So if that feature bit is set in XFD, you're clearing it here. Why?
> 
> So that it doesn't raise that #NM for it anymore?
> 
> This looks weird.

If this ever happens, something might be wrong with the hardware.

If the bit is not reset, it will get stuck with repeatedly unhandled #NMs,
which I think is even more terrible.

>> +			} else {
>> +				struct fpu *fpu = &current->thread.fpu;
>> +				int err = -1;
>> +
>> +				/*
>> +				 * Make sure not in interrupt context as handling a
>> +				 * trap from userspace.
>> +				 */
>> +				if (!WARN_ON(in_interrupt())) {
> 
> I'm guessing that's supposed to stop people from using AMX and other
> dynamic states in the kernel?

But the kernel can handle this without XFD?

Thanks,
Chang