Re: [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()
On 2018-11-09 19:52:02 [+0100], Borislav Petkov wrote:
> On Fri, Nov 09, 2018 at 06:35:21PM +0100, Sebastian Andrzej Siewior wrote:
> > fpu__drop() stets ->initialized to 0. As a result the context switch
>
> "... the context switch path landing in switch_fpu_prepare()... " is what you
> mean, right?
I mean both. switch_fpu_prepare() while the task is leaving and then
switch_fpu_finish() while the task is coming back. But yes.
> > 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)
>
> You mean, the FPU regs will have random values, yes.
correct. Same like for kernel threads.
> > after the context switch. This is also true for usage
> > in softirq via kernel_fpu_begin().
>
> So far so good.
>
> Except maybe because I'm dense about FPU, I still am missing something.
>
> You have this path:
>
> __fpu__restore_sig
> |-> fpu__clear
> |-> fpu__drop
>
> and that happens on the sigreturn() path.
>
> Now, the context switch happens ... when exactly?
>
> After the sigreturn is done?
Is fpu__clear() correct here? If so, a context switch after setting
->initialized has been set to 1 wouldn't matter because in the end the
register state is restored from init_fpstate and not from task's FPU
struct.
>
> It must be because then you'd get that ->state corruption after
> ->initialized has been cleared.
>
> Right?
I might got your question wrong. If you quote the code and try again and
I do so, too :)
>
>
> > 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, , 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…
>
> Funny, you should mention that.
>
> But this very much rings a bell about a very elusive bug we had on
> 32-bit at the time. See attached mbox (yeah, the web archives were crap
> and couldn't find the links so I'm sending you the whole thread).
>
> And at the time Ingo said that there's something still missing about
> *why* it would happen.
>
> And I think it is this context switch happening right after the
> sigreturn - *AFAICT* - which would cause this.
>
> I could very well be off but this smells very similar to your thing.
So checking out v4.5-rc3-15-g58122bf1d856a and __fpu__restore_sig() is
something like this:
| fpu__drop(fpu);
…
| fpu->fpstate_active = 1;
X
| if (use_eager_fpu()) {
| preempt_disable();
| fpu__restore(fpu);
| preempt_enable();
| }
fpu__drop() sets fpstate_active & fpregs_active to 0[¹]. A context switch
at X would _not_ save current FPU registers and overwrite what was
prepared because fpregs_active should still be zero.
Now on the switch back to the task, fpstate_active was set which means
fpu.preload might be true. If so it would load the FPU registers and set
fpregs_active to 1. Later fpu__restore() would try the same and
fpregs_activate() would trigger the warning because fpregs_active was
already set to 1.
> Hmmm.
So I just came up with a possible hard to trigger case and a robot
triggered it already a while back. Well, CONFIG_PREEMPT=y is also there
so it matches this part of the story. But you connected the dots.
[¹] side note: in my early research it took a while to notice that
fpstate_active and fpregs_active were two different things. My brain
used fp.*_active for matching. It also helped my confusion that
those were renamed and removed…
Sebastian
Re: [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()
On 2018-11-09 19:52:02 [+0100], Borislav Petkov wrote:
> On Fri, Nov 09, 2018 at 06:35:21PM +0100, Sebastian Andrzej Siewior wrote:
> > fpu__drop() stets ->initialized to 0. As a result the context switch
>
> "... the context switch path landing in switch_fpu_prepare()... " is what you
> mean, right?
I mean both. switch_fpu_prepare() while the task is leaving and then
switch_fpu_finish() while the task is coming back. But yes.
> > 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)
>
> You mean, the FPU regs will have random values, yes.
correct. Same like for kernel threads.
> > after the context switch. This is also true for usage
> > in softirq via kernel_fpu_begin().
>
> So far so good.
>
> Except maybe because I'm dense about FPU, I still am missing something.
>
> You have this path:
>
> __fpu__restore_sig
> |-> fpu__clear
> |-> fpu__drop
>
> and that happens on the sigreturn() path.
>
> Now, the context switch happens ... when exactly?
>
> After the sigreturn is done?
Is fpu__clear() correct here? If so, a context switch after setting
->initialized has been set to 1 wouldn't matter because in the end the
register state is restored from init_fpstate and not from task's FPU
struct.
>
> It must be because then you'd get that ->state corruption after
> ->initialized has been cleared.
>
> Right?
I might got your question wrong. If you quote the code and try again and
I do so, too :)
>
>
> > 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, , 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…
>
> Funny, you should mention that.
>
> But this very much rings a bell about a very elusive bug we had on
> 32-bit at the time. See attached mbox (yeah, the web archives were crap
> and couldn't find the links so I'm sending you the whole thread).
>
> And at the time Ingo said that there's something still missing about
> *why* it would happen.
>
> And I think it is this context switch happening right after the
> sigreturn - *AFAICT* - which would cause this.
>
> I could very well be off but this smells very similar to your thing.
So checking out v4.5-rc3-15-g58122bf1d856a and __fpu__restore_sig() is
something like this:
| fpu__drop(fpu);
…
| fpu->fpstate_active = 1;
X
| if (use_eager_fpu()) {
| preempt_disable();
| fpu__restore(fpu);
| preempt_enable();
| }
fpu__drop() sets fpstate_active & fpregs_active to 0[¹]. A context switch
at X would _not_ save current FPU registers and overwrite what was
prepared because fpregs_active should still be zero.
Now on the switch back to the task, fpstate_active was set which means
fpu.preload might be true. If so it would load the FPU registers and set
fpregs_active to 1. Later fpu__restore() would try the same and
fpregs_activate() would trigger the warning because fpregs_active was
already set to 1.
> Hmmm.
So I just came up with a possible hard to trigger case and a robot
triggered it already a while back. Well, CONFIG_PREEMPT=y is also there
so it matches this part of the story. But you connected the dots.
[¹] side note: in my early research it took a while to notice that
fpstate_active and fpregs_active were two different things. My brain
used fp.*_active for matching. It also helped my confusion that
those were renamed and removed…
Sebastian
Re: [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()
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, , 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
Re: [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()
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, , 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
Re: [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()
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. 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. 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. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 02/23] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()
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. 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. 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. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
