On 2018-11-08 15:57:21 [+0100], Borislav Petkov wrote:
> On Wed, Nov 07, 2018 at 08:48:37PM +0100, Sebastian Andrzej Siewior wrote:
> > This is a preparation for the removal of the ->initialized member in the
> > fpu struct.
> > __fpu__restore_sig() is deactivating the FPU via fpu__drop() and then
> > setting manually ->initialized followed by fpu__restore(). The result is
> > that it is possible to manipulate fpu->state and the state of registers
> > won't be saved/restore on a context switch which would overwrite state.
>
> restored
>
> >
> > Don't access the fpu->state while the content is read from user space
> > and examined / sanitized. Use a temporary buffer kmalloc() buffer for
>
> one "buffer" too many.
corrected.
> More importantly, what I'm missing here is more detailed explanation
> about how that manipulation can happen. Especially since the comment
> over fpu__drop() you're removing below is claiming the exact opposite.
> AFAICT.
fpu__drop() stets ->initialized to 0. As a result the context switch
will not save current FPU registers and so _not_ write to fpu->state.
This also means that CPU's FPU register will be random (inherited from
the last context) after the context switch. This is also true for usage
in softirq via kernel_fpu_begin().
The "new" FPU state is prepared in fpu->state and once it is done, it
gets loaded via
fpu->initialized = 1; // make sure fpu__initialize() in fpu__restore()
// is a nop
fpu__restore(); // Load the registers.
Since I plan to remove the ->initialized member, I don't have the luxury
to play with fpu->state because the "new" content obtained by
copy_from_user() will be overwritten with CPU's current FPU state during
a context switch.
Now with that information, what do you plan to alter? :)
> Yeah, FPU code has always been nasty and tricky to follow so I think
> we'd need to have this stuff explained in much more detail.
Yeah, tell me about it. Now that you made me look into this again, I
spotted this gem:
| __fpu__restore_sig()
…
| if (ia32_fxstate) {
…
| fpu__drop(fpu);
…
| /* prepare new FPU state in fpu->state */
|
| fpu->initialized = 1;
*BOOM* context switch. ->initialized == 1 is seen so it stashes current
CPU's FPU state into fpu->state and overwrites what has been prepared
before.
On the switch back to this task, the fpu__restore() becomes a "nop"
because the saved registers are the same but not what was expected /
prepared before.
| preempt_disable();
| fpu__restore(fpu);
| preempt_enable();
|
So. The fix would be:
@@ -344,10 +344,10 @@ static int __fpu__restore_sig(void __user *buf, void
__user *buf_fx, int size)
sanitize_restored_xstate(tsk, &env, xfeatures, fx_only);
}
+ local_bh_disable();
fpu->initialized = 1;
- preempt_disable();
fpu__restore(fpu);
- preempt_enable();
+ local_bh_enable();
return err;
} else {
local_bh_disable() due to possible kernel_fpu_begin() usage in softirq.
How much do we care here about a theoretical race on 32bit anyway? I
don't think someone complained :) I would have to rebase my queue…
otherwise…
> Thx.
Sebastian
