[PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Sebastian Andrzej Siewior]

The sequence
   fpu->initialized = 1; /* step A */
   preempt_disable();	 /* step B */
   fpu__restore(fpu);
   preempt_enable();

is racy in regard to a context switch.
For 32bit frames __fpu__restore_sig() prepares the FPU state within
fpu->state. To ensure that a context switch (switch_fpu_prepare() in
particular) does not modify fpu->state it uses fpu__drop() which sets
fpu->initializes to 0. With this change the CPU's FPU state is not saved
to fpu->state during a context switch. It then loads the state to
fpu->state from userland and ensures it sane. The new state is loaded
via fpu__restore(). The code sets then fpu->initializes to 1 in order
to avoid fpu__initialize() doing anything (overwrite the new state)
which is part of fpu__restore().
A context switch between step A and B would save CPU's current FPU
registers to fpu->state and overwrite the newly prepared state. This
looks like tiny race window but the Kernel Test Robot reported this back
in 2016 while we had lazy FPU support. Borislav Petkov made the link
between that report and another patch that has been posted.
Since the removal of the lazy FPU support, this race goes unnoticed
because the warning has been removed.

Use local_bh_disable() around the restore sequence to avoid the race. BH
needs to be disabled because BH is allowed to run (even with preemption
disabled) and might invoke kernel_fpu_begin().

Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic
Cc: stable@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: A more verbose commit as message.

 arch/x86/kernel/fpu/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 61a949d84dfa5..d99a8ee9e185e 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -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 {
-- 
2.19.1

Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Dave Hansen]

On 11/19/18 8:04 AM, Sebastian Andrzej Siewior wrote:
> v1…v2: A more verbose commit as message.

I was really hoping for code comments. :)

Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Sebastian Andrzej Siewior]

On 2018-11-19 09:02:45 [-0800], Dave Hansen wrote:
> On 11/19/18 8:04 AM, Sebastian Andrzej Siewior wrote:
> > v1…v2: A more verbose commit as message.
> 
> I was really hoping for code comments. :)

I though we agreed to make those in the larger series because those
comments in __fpu__restore_sig() would be removed anyway (as part of the
series).

Sebastian

Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Borislav Petkov]

On Mon, Nov 19, 2018 at 06:11:29PM +0100, Sebastian Andrzej Siewior wrote:
> On 2018-11-19 09:02:45 [-0800], Dave Hansen wrote:
> > On 11/19/18 8:04 AM, Sebastian Andrzej Siewior wrote:
> > > v1…v2: A more verbose commit as message.
> > 
> > I was really hoping for code comments. :)
> 
> I though we agreed to make those in the larger series because those
> comments in __fpu__restore_sig() would be removed anyway (as part of the
> series).

Also, over local_bh_disable() does not really fit as this is generic
code and Sebastian said ARM does the whole thing a bit differently, for
one.

We probably should put that comment somewhere prominent in
arch/x86/kernel/fpu/core.c or similar - somewhere people poking at FPU
stuff will see it.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Sebastian Andrzej Siewior]

On 2018-11-19 18:27:43 [+0100], Borislav Petkov wrote:
> On Mon, Nov 19, 2018 at 06:11:29PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2018-11-19 09:02:45 [-0800], Dave Hansen wrote:
> > > On 11/19/18 8:04 AM, Sebastian Andrzej Siewior wrote:
> > > > v1…v2: A more verbose commit as message.
> > > 
> > > I was really hoping for code comments. :)
> > 
> > I though we agreed to make those in the larger series because those
> > comments in __fpu__restore_sig() would be removed anyway (as part of the
> > series).
> 
> Also, over local_bh_disable() does not really fit as this is generic
> code and Sebastian said ARM does the whole thing a bit differently, for
> one.
> 
> We probably should put that comment somewhere prominent in
> arch/x86/kernel/fpu/core.c or similar - somewhere people poking at FPU
> stuff will see it.

I though about __fpregs_changes_begin() in the last patch of the commit:
 https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/commit/?h=x86_fpu_rtu_v4

Sebastian

Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Dave Hansen]

On 11/19/18 9:27 AM, Borislav Petkov wrote:
>>> I was really hoping for code comments. :)
>> I though we agreed to make those in the larger series because those
>> comments in __fpu__restore_sig() would be removed anyway (as part of the
>> series).
> Also, over local_bh_disable() does not really fit as this is generic
> code and Sebastian said ARM does the whole thing a bit differently, for
> one.
> 
> We probably should put that comment somewhere prominent in
> arch/x86/kernel/fpu/core.c or similar - somewhere people poking at FPU
> stuff will see it.

Ahh, got it.

arch/x86/kernel/fpu/core.c would be a fine spot.

Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Borislav Petkov]

On Mon, Nov 19, 2018 at 06:31:36PM +0100, Sebastian Andrzej Siewior wrote:
> I though about __fpregs_changes_begin() in the last patch of the commit:
>  https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/commit/?h=x86_fpu_rtu_v4

Also a good spot - api.h talks about preemption already so sure, why not.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Borislav Petkov]

On Mon, Nov 19, 2018 at 05:04:10PM +0100, Sebastian Andrzej Siewior wrote:
> The sequence
>    fpu->initialized = 1; /* step A */
>    preempt_disable();	 /* step B */
>    fpu__restore(fpu);
>    preempt_enable();
> 
> is racy in regard to a context switch.
> For 32bit frames __fpu__restore_sig() prepares the FPU state within
> fpu->state. To ensure that a context switch (switch_fpu_prepare() in
> particular) does not modify fpu->state it uses fpu__drop() which sets
> fpu->initializes to 0.

"... ->initialized to 0."

Also, a new line here pls.

> With this change the CPU's FPU state is not saved
		  ^

comma:

		 ,

Also, instead of "with this change" I think you mean: "After
->initialized is cleared, the CPU's FPU state..."

> to fpu->state during a context switch.
> It then loads the state to fpu->state from userland and ensures it
> sane.

"... and ensures it is sane."

> The new state is loaded via fpu__restore(). The code sets then
> fpu->initializes to 1 in order to avoid fpu__initialize() doing

fpu->initialized

> anything (overwrite the new state) which is part of fpu__restore().

<---- newline here.

> A context switch between step A and B would save CPU's current FPU
> registers to fpu->state and overwrite the newly prepared state. This
> looks like tiny race window but the Kernel Test Robot reported this back
> in 2016 while we had lazy FPU support. Borislav Petkov made the link
> between that report and another patch that has been posted.
> Since the removal of the lazy FPU support, this race goes unnoticed
> because the warning has been removed.
> 
> Use local_bh_disable() around the restore sequence to avoid the race. BH

Let's write it out once: "Bottom halves need to be... "

> needs to be disabled because BH is allowed to run (even with preemption
> disabled) and might invoke kernel_fpu_begin().

... and let's put the potential example here with IPsec and softirq.

> Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic
> Cc: stable@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2: A more verbose commit as message.

Very much needed, thanks!

>  arch/x86/kernel/fpu/signal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 61a949d84dfa5..d99a8ee9e185e 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -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 {
> -- 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v3] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig()

[Sebastian Andrzej Siewior]

The sequence
   fpu->initialized = 1; /* step A */
   preempt_disable();	 /* step B */
   fpu__restore(fpu);
   preempt_enable();

is racy in regard to a context switch.
For 32bit frames __fpu__restore_sig() prepares the FPU state within
fpu->state. To ensure that a context switch (switch_fpu_prepare() in
particular) does not modify fpu->state it uses fpu__drop() which sets
fpu->initialized to 0.
After fpu->initialized is cleared, the CPU's FPU state is not saved to
fpu->state during a context switch. It then loads the new state to
fpu->state from userland and ensures it is sane. The new state is loaded
via fpu__restore(). The code sets then fpu->initializes to 1 in order to
avoid fpu__initialize() doing anything (overwrite the new state) which
is part of fpu__restore().

A context switch between step A and B would save CPU's current FPU
registers to fpu->state and overwrite the newly prepared state. This
looks like tiny race window but the Kernel Test Robot reported this back
in 2016 while we had lazy FPU support. Borislav Petkov made the link
between that report and another patch that has been posted.
Since the removal of the lazy FPU support, this race goes unnoticed
because the warning has been removed.

Disable bottom halfes around the restore sequence to avoid the race. BH
needs to be disabled because BH is allowed to run (even with preemption
disabled) and might invoke kernel_fpu_begin() by doing IPsec.

Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic
Cc: stable@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3: Rewording parts of the commit message as per Borislav Petkov.
v1…v2: A more verbose commit as message.

 arch/x86/kernel/fpu/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 61a949d84dfa5..d99a8ee9e185e 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -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 {
-- 
2.19.1

[tip:x86/urgent] x86/fpu: Disable bottom halves while loading FPU registers

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  265fb8fe4c69a94431a17e8b87a9613d30fefe79
Gitweb:     https://git.kernel.org/tip/265fb8fe4c69a94431a17e8b87a9613d30fefe79
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Tue, 20 Nov 2018 11:26:35 +0100
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Tue, 20 Nov 2018 13:01:32 +0100

x86/fpu: Disable bottom halves while loading FPU registers

The sequence

  fpu->initialized = 1;		/* step A */
  preempt_disable();		/* step B */
  fpu__restore(fpu);
  preempt_enable();

in __fpu__restore_sig() is racy in regard to a context switch.

For 32bit frames, __fpu__restore_sig() prepares the FPU state within
fpu->state. To ensure that a context switch (switch_fpu_prepare() in
particular) does not modify fpu->state it uses fpu__drop() which sets
fpu->initialized to 0.

After fpu->initialized is cleared, the CPU's FPU state is not saved
to fpu->state during a context switch. The new state is loaded via
fpu__restore(). It gets loaded into fpu->state from userland and
ensured it is sane. fpu->initialized is then set to 1 in order to avoid
fpu__initialize() doing anything (overwrite the new state) which is part
of fpu__restore().

A context switch between step A and B above would save CPU's current FPU
registers to fpu->state and overwrite the newly prepared state. This
looks like a tiny race window but the Kernel Test Robot reported this
back in 2016 while we had lazy FPU support. Borislav Petkov made the
link between that report and another patch that has been posted. Since
the removal of the lazy FPU support, this race goes unnoticed because
the warning has been removed.

Disable bottom halves around the restore sequence to avoid the race. BH
need to be disabled because BH is allowed to run (even with preemption
disabled) and might invoke kernel_fpu_begin() by doing IPsec.

 [ bp: massage commit message a bit. ]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@kernel.org>
CC: Dave Hansen <dave.hansen@linux.intel.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: "Jason A. Donenfeld" <Jason@zx2c4.com>
CC: kvm ML <kvm@vger.kernel.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Radim Krčmář <rkrcmar@redhat.com>
CC: Rik van Riel <riel@surriel.com>
Cc: stable@vger.kernel.org
CC: x86-ml <x86@kernel.org>
Link: http://lkml.kernel.org/r/20181120102635.ddv3fvavxajjlfqk@linutronix.de
Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic
---
 arch/x86/kernel/fpu/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 61a949d84dfa..d99a8ee9e185 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -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 {

[tip:x86/urgent] x86/fpu: Disable bottom halves while loading FPU registers

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  68239654acafe6aad5a3c1dc7237e60accfebc03
Gitweb:     https://git.kernel.org/tip/68239654acafe6aad5a3c1dc7237e60accfebc03
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Tue, 20 Nov 2018 11:26:35 +0100
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Tue, 20 Nov 2018 17:22:42 +0100

x86/fpu: Disable bottom halves while loading FPU registers

The sequence

  fpu->initialized = 1;		/* step A */
  preempt_disable();		/* step B */
  fpu__restore(fpu);
  preempt_enable();

in __fpu__restore_sig() is racy in regard to a context switch.

For 32bit frames, __fpu__restore_sig() prepares the FPU state within
fpu->state. To ensure that a context switch (switch_fpu_prepare() in
particular) does not modify fpu->state it uses fpu__drop() which sets
fpu->initialized to 0.

After fpu->initialized is cleared, the CPU's FPU state is not saved
to fpu->state during a context switch. The new state is loaded via
fpu__restore(). It gets loaded into fpu->state from userland and
ensured it is sane. fpu->initialized is then set to 1 in order to avoid
fpu__initialize() doing anything (overwrite the new state) which is part
of fpu__restore().

A context switch between step A and B above would save CPU's current FPU
registers to fpu->state and overwrite the newly prepared state. This
looks like a tiny race window but the Kernel Test Robot reported this
back in 2016 while we had lazy FPU support. Borislav Petkov made the
link between that report and another patch that has been posted. Since
the removal of the lazy FPU support, this race goes unnoticed because
the warning has been removed.

Disable bottom halves around the restore sequence to avoid the race. BH
need to be disabled because BH is allowed to run (even with preemption
disabled) and might invoke kernel_fpu_begin() by doing IPsec.

 [ bp: massage commit message a bit. ]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: kvm ML <kvm@vger.kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Rik van Riel <riel@surriel.com>
Cc: stable@vger.kernel.org
Cc: x86-ml <x86@kernel.org>
Link: http://lkml.kernel.org/r/20181120102635.ddv3fvavxajjlfqk@linutronix.de
Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic
---
 arch/x86/kernel/fpu/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 61a949d84dfa..d99a8ee9e185 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -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 {