Re: [PATCH] x86/fpu: verify xstate buffer size according with requested features

[Thomas Gleixner]

On Wed, Jan 17 2024 at 23:59, Andrei Vagin wrote:
> On Wed, Jan 17, 2024 at 3:52 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>> I'm not really following the logic there.  What's the downside of taking
>> the fault?
>
> Let's consider a scenario where someone messed up with an fpu state on a
> signal frame.

Then he can rightfully keep the pieces...

> With my approach, a mistake can be promptly detected.

How so? Everything which ends up at the 'setfx:' label will just
silently fall back to FX only and init all other components.

> However, if we incorporate the page fault handling of xrstor, a
> mistake will only be identified if xrstor triggers a fault. In cases
> where a buffer is allocated in a large memory mapping, xrstor may
> silently read memory beyond the buffer.

It's either failing the restore due to invalid data (#GP) or it will
restore garbage. User space asked for it.

> Next time, a page beyond a buffer might be swapped out, xrstore
> triggers a fault leading to application crashes.

If it's swapped out it will be swapped back in, no crash.

There are two ways for crashing:

      1) There is no mapping or a non-sufficient mapping
         i.e. fault_in_readable() fails.

      2) The data in the buffer is invalid.

Crashing the application in both cases is just fine.

The nasty part is that the expected size of the user space buffer is
taken from fx_sw->xstate_size.

So you can construct a sigreturn frame where

   1) fx_sw->xstate_size is smaller than the size required by
      the valid bits in fx_sw->xfeatures.

   2) user space unmapped parts of the stack so that not all
      of the buffer (as required by XRSTOR) is accessible.

Now XRSTOR tries to restore and accesses the unmapped part of the stack,
which results in a fault. But fault_in_readable() succeeds because 'buf
+ fx_sw->xstate_size' is within the still mapped stack.

So it goes back and tries XRSTOR again. Lather, rinse and repeat.

That's what Andrej is trying to prevent by calculating the size required
by the valid bits in fx_sw->xfeatures and validating that against
fx_sw->xstate_size.

That fx_sw construct is yet another horror from the past. It's not much
better than xsave itself.

Thanks,

        tglx