Re: [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()

[Sebastian Andrzej Siewior]

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