Messages in this thread | | | From | "Bae, Chang Seok" <> | Subject | Re: [PATCH v9 12/26] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state | Date | Wed, 18 Aug 2021 19:47:49 +0000 |
| |
[ 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 = ¤t->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
| |