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

[Dave Hansen]

On 1/17/24 23:59, Andrei Vagin wrote:
> On Wed, Jan 17, 2024 at 3:52 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
..
>> "fx_sw" is actually a software-defined and software-only-consumed area
>> of the XSAVE buffer, thus the '_sw'.  Nothing in the '_sw' section tells
>> us how the hardware will behave.
> 
> I think you don't take into account the requested-feature bitmap (RFBM),
> which is the logical-and of edx:eax and XCR0. In our case, edx:eax
> is set to the value of fx_sw->xfeatures.

Ahh, I did miss that indeed.

Let's step back and look at what's in play:

1. fx_sw->xstate_size, which is eventually passed to
   fault_in_readable()
2. fx_sw->xfeatures
3. XSTATE_BV (aka. fpu->...->header.xfeatures)
4. What XRSTOR actually does, which is #2 OR'd with #3.
5. xstate_calculate_size(fx_sw->xfeatures)

The bug that you've reported here is essentially that the size passed to
fault_in_readable() doesn't match what XRSTOR actually does (#4).   Note
that today, fault_in_readable() may end up faulting in _too_ much memory
if there's a bit clear in XSTATE_BV.

The proposed fix adds the #5 calculation.  It's conservative because
fx_sw->xfeatures is represents a superset of what XRSTOR will actually
restore.  But now, instead of just faulting in too much memory, a
too-small fx_sw->xstate_size will end up zapping all the XSAVE state.

This is all freakishly complicated and changes a bunch of behavior.
It's way too much to be done in a patch with a 5-line commit message.

I suspect this needs some real refactoring.  I really think fx_sw should
remain unmodified after being copied in from userspace.  If you want to
start interpreting 'fx_sw->xstate_size' as the fault_in_readable() size,
then that needs to be a separate logical variable.

>>>> because the CPU knows where the fault happened.  It told us in CR2 and
>>>> all we have to do is plumb that back to fault_in_readable().
>>>
>>> I considered this option as well, but then I decided that this approach
>>> is better. The most important aspect is that it always rejects bad
>>> buffers, allowing a user space to detect an issue even when a fault
>>> isn't triggered. I believe proper handling of xrstor page faults could
>>> be a valuable additional improvement to this change. If we detect a
>>> fault outside of a provided buffer, we can print a warning to signal
>>> that check_xstate_in_sigframe is incomplete.
>>
>> 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. With my approach, a mistake can be promptly detected.
> 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. Next time, a page beyond a buffer might be
> swapped out, xrstore triggers a fault leading to application crashes.

I think that's an orthogonal problem really.

Fault loops are nasty.  There's a reason that the architecture provides
CR2 instead of depending on software to, for instance, figure out why
and how every instruction faulted.  It's easy to look into the past and
as the CPU where the fault happened.

On the other hand, we have XRSTOR.  Sure, it's _possible_ to look into
the future and figure out what memory XRSTOR will touch.  But, we
apparently stink at looking into the future (thus this bug).  Not
because we're stupid, but simply because looking into the future is hard.

I'd much rather fix the fault loop problem by looking into the past than
predicting the future.  The fault handling _must_ be correct or we get
hangs.

If we have nice, reliable fault handling and then decide that we've got
XRSTOR's running amok reading random memory all over the place that need
a nicer error message, then we can add that code to predict the future.
If our "predict the future" code goes wrong, then we lose an error
message -- not a big deal.