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

Bae, Chang Seok
2021-08-18T19:48:03Z

[ 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