[1/2] arm64: fpsimd: Fix FPSIMD corruption in rt_sigreturn with CONFIG_PREEMPT

Message ID 1442316997-32282-1-git-send-email-will.deacon@arm.com
State New
Headers show

Commit Message

Will Deacon Sept. 15, 2015, 11:36 a.m.
From: Dave P Martin <Dave.Martin@arm.com>

The arm64 context switch implementation uses a flag
TIF_FOREIGN_FPSTATE to track whether the hardware FPSIMD regs are
out of sync with the logical state of current's registers.

During sigreturn, the registers and task_struct are temporarily out
of sync, between writing the task_struct and loading its contents
back into the FPSIMD registers -- however, TIF_FOREIGN_FPSTATE is
not set.  This can cause the context switch code to discard some or
all of the restored FPSIMD state if preemption occurs during the
critical region of rt_sigreturn.

This patch sets TIF_FOREIGN_FPSTATE before transferring the
sigframe's saved registers back to the task_struct, so that the
task_struct data will take precedence over the hardware registers
if a context switch occurs before everything is back in sync.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
[will: removed preempt_{enable,disable} calls, added compat version]
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/signal.c   | 3 +++
 arch/arm64/kernel/signal32.c | 2 ++
 2 files changed, 5 insertions(+)

Comments

Ard Biesheuvel Sept. 15, 2015, 12:11 p.m. | #1
Hi Will,

On 15 September 2015 at 13:36, Will Deacon <will.deacon@arm.com> wrote:
> From: Dave P Martin <Dave.Martin@arm.com>
>
> The arm64 context switch implementation uses a flag
> TIF_FOREIGN_FPSTATE to track whether the hardware FPSIMD regs are
> out of sync with the logical state of current's registers.
>
> During sigreturn, the registers and task_struct are temporarily out
> of sync, between writing the task_struct and loading its contents
> back into the FPSIMD registers -- however, TIF_FOREIGN_FPSTATE is
> not set.  This can cause the context switch code to discard some or
> all of the restored FPSIMD state if preemption occurs during the
> critical region of rt_sigreturn.
>
> This patch sets TIF_FOREIGN_FPSTATE before transferring the
> sigframe's saved registers back to the task_struct, so that the
> task_struct data will take precedence over the hardware registers
> if a context switch occurs before everything is back in sync.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> [will: removed preempt_{enable,disable} calls, added compat version]
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/signal.c   | 3 +++
>  arch/arm64/kernel/signal32.c | 2 ++
>  2 files changed, 5 insertions(+)
>
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index e18c48cb6db1..6d50d839b6e9 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -79,6 +79,9 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
>         if (magic != FPSIMD_MAGIC || size != sizeof(struct fpsimd_context))
>                 return -EINVAL;
>
> +       /* Ensure we don't reload stale data from the hardware registers */
> +       set_ti_thread_flag(current_thread_info(), TIF_FOREIGN_FPSTATE);
> +
>         /* copy the FP and status/control registers */
>         err = __copy_from_user(fpsimd.vregs, ctx->vregs,
>                                sizeof(fpsimd.vregs));

I am not following something here. If the task gets preempted before
the call to fpsimd_update_current_state(), it will proceed to
overwrite the registers with whatever is on the stack in fpsimd after
being scheduled back in, and everything should work fine, right?. So
the problem must lie /after/ (or during) the call to
fpsimd_update_current_state(), which does the following

void fpsimd_update_current_state(struct fpsimd_state *state)
{
        preempt_disable();
        fpsimd_load_state(state);
        if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
                struct fpsimd_state *st = &current->thread.fpsimd_state;

                this_cpu_write(fpsimd_last_state, st);
                st->cpu = smp_processor_id();
        }
        preempt_enable();
}

i.e., it copies the FP/SIMD state into the registers, and then marks
the h/w state of this cpu as the most recent for current.

It is not clear to me at which point the preemption is hitting when it
causes this problem.

Other than that, I think it may be more appropriate to call
fpsimd_flush_task_state() to invalidate any in-register copies of the
task's FP/SIMD state rather than setting the TIF flag directly (which
is normally only used as a shorthand to indicate that this cpu's
fpsimd_last_state and this tasks fpsimd_state::cpu are out of sync,
and set automatically by the context switch code)
Will Deacon Sept. 15, 2015, 2:39 p.m. | #2
On Tue, Sep 15, 2015 at 01:11:10PM +0100, Ard Biesheuvel wrote:
> Hi Will,

Hi Ard,

Thanks for having a look.

> On 15 September 2015 at 13:36, Will Deacon <will.deacon@arm.com> wrote:
> > From: Dave P Martin <Dave.Martin@arm.com>
> >
> > The arm64 context switch implementation uses a flag
> > TIF_FOREIGN_FPSTATE to track whether the hardware FPSIMD regs are
> > out of sync with the logical state of current's registers.
> >
> > During sigreturn, the registers and task_struct are temporarily out
> > of sync, between writing the task_struct and loading its contents
> > back into the FPSIMD registers -- however, TIF_FOREIGN_FPSTATE is
> > not set.  This can cause the context switch code to discard some or
> > all of the restored FPSIMD state if preemption occurs during the
> > critical region of rt_sigreturn.
> >
> > This patch sets TIF_FOREIGN_FPSTATE before transferring the
> > sigframe's saved registers back to the task_struct, so that the
> > task_struct data will take precedence over the hardware registers
> > if a context switch occurs before everything is back in sync.
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > [will: removed preempt_{enable,disable} calls, added compat version]
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm64/kernel/signal.c   | 3 +++
> >  arch/arm64/kernel/signal32.c | 2 ++
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > index e18c48cb6db1..6d50d839b6e9 100644
> > --- a/arch/arm64/kernel/signal.c
> > +++ b/arch/arm64/kernel/signal.c
> > @@ -79,6 +79,9 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
> >         if (magic != FPSIMD_MAGIC || size != sizeof(struct fpsimd_context))
> >                 return -EINVAL;
> >
> > +       /* Ensure we don't reload stale data from the hardware registers */
> > +       set_ti_thread_flag(current_thread_info(), TIF_FOREIGN_FPSTATE);
> > +
> >         /* copy the FP and status/control registers */
> >         err = __copy_from_user(fpsimd.vregs, ctx->vregs,
> >                                sizeof(fpsimd.vregs));
> 
> I am not following something here. If the task gets preempted before
> the call to fpsimd_update_current_state(), it will proceed to
> overwrite the registers with whatever is on the stack in fpsimd after
> being scheduled back in, and everything should work fine, right?.

You're completely right, sorry for wasting your time. Dave and I have
been working on the fpsimd code recently and have another patch that
restores the state directly into the current thread to avoid the stack
entirely. *That* is what caused this problem, and I incorrectly
identified this as a fix for mainline (it's extremely similar to
2af276dfb172 ("ARM: 7306/1: vfp: flush thread hwstate before restoring
context from sigframe"), which continues to haunt me).

Anyway, that also explains why we hadn't seen any problems in our
previous testing! The second patch (compat big-endian fix) still stands,
however.

Will
Dave Martin Sept. 21, 2015, 3:37 p.m. | #3
On Tue, Sep 15, 2015 at 03:39:53PM +0100, Will Deacon wrote:
> On Tue, Sep 15, 2015 at 01:11:10PM +0100, Ard Biesheuvel wrote:
> > Hi Will,
> 
> Hi Ard,
> 
> Thanks for having a look.
> 
> > On 15 September 2015 at 13:36, Will Deacon <will.deacon@arm.com> wrote:
> > > From: Dave P Martin <Dave.Martin@arm.com>
> > >
> > > The arm64 context switch implementation uses a flag
> > > TIF_FOREIGN_FPSTATE to track whether the hardware FPSIMD regs are
> > > out of sync with the logical state of current's registers.
> > >
> > > During sigreturn, the registers and task_struct are temporarily out
> > > of sync, between writing the task_struct and loading its contents
> > > back into the FPSIMD registers -- however, TIF_FOREIGN_FPSTATE is
> > > not set.  This can cause the context switch code to discard some or
> > > all of the restored FPSIMD state if preemption occurs during the
> > > critical region of rt_sigreturn.
> > >
> > > This patch sets TIF_FOREIGN_FPSTATE before transferring the
> > > sigframe's saved registers back to the task_struct, so that the
> > > task_struct data will take precedence over the hardware registers
> > > if a context switch occurs before everything is back in sync.
> > >
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > [will: removed preempt_{enable,disable} calls, added compat version]
> > > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > > ---
> > >  arch/arm64/kernel/signal.c   | 3 +++
> > >  arch/arm64/kernel/signal32.c | 2 ++
> > >  2 files changed, 5 insertions(+)
> > >
> > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > > index e18c48cb6db1..6d50d839b6e9 100644
> > > --- a/arch/arm64/kernel/signal.c
> > > +++ b/arch/arm64/kernel/signal.c
> > > @@ -79,6 +79,9 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
> > >         if (magic != FPSIMD_MAGIC || size != sizeof(struct fpsimd_context))
> > >                 return -EINVAL;
> > >
> > > +       /* Ensure we don't reload stale data from the hardware registers */
> > > +       set_ti_thread_flag(current_thread_info(), TIF_FOREIGN_FPSTATE);
> > > +
> > >         /* copy the FP and status/control registers */
> > >         err = __copy_from_user(fpsimd.vregs, ctx->vregs,
> > >                                sizeof(fpsimd.vregs));
> > 
> > I am not following something here. If the task gets preempted before
> > the call to fpsimd_update_current_state(), it will proceed to
> > overwrite the registers with whatever is on the stack in fpsimd after
> > being scheduled back in, and everything should work fine, right?.
> 
> You're completely right, sorry for wasting your time. Dave and I have
> been working on the fpsimd code recently and have another patch that
> restores the state directly into the current thread to avoid the stack
> entirely. *That* is what caused this problem, and I incorrectly
> identified this as a fix for mainline (it's extremely similar to
> 2af276dfb172 ("ARM: 7306/1: vfp: flush thread hwstate before restoring
> context from sigframe"), which continues to haunt me).

Urg, looks like I confused myself here.  Ard's assessment looks correct.

Not staging the data via the stack seemed a harmless change which I
thought was an improvement, since at half a K the amount of data is non-
trivial.

I'll go back and make some measurements to see whether getting rid
of the extra copy is really worth it.  I think reading straight into
the task_struct with __get_user() can work, but it looks like I'll
need to fiddle with the preemption region, or otherwise handle the
ensuing race...

> Anyway, that also explains why we hadn't seen any problems in our
> previous testing! The second patch (compat big-endian fix) still stands,
> however.

I misidentified the problem as being independent of my hacks --
apologies for the confusion there.

Cheers
---Dave
Ard Biesheuvel Sept. 23, 2015, 1:20 a.m. | #4
On 21 September 2015 at 08:37, Dave P Martin <Dave.Martin@arm.com> wrote:
> On Tue, Sep 15, 2015 at 03:39:53PM +0100, Will Deacon wrote:
>> On Tue, Sep 15, 2015 at 01:11:10PM +0100, Ard Biesheuvel wrote:
>> > Hi Will,
>>
>> Hi Ard,
>>
>> Thanks for having a look.
>>
>> > On 15 September 2015 at 13:36, Will Deacon <will.deacon@arm.com> wrote:
>> > > From: Dave P Martin <Dave.Martin@arm.com>
>> > >
>> > > The arm64 context switch implementation uses a flag
>> > > TIF_FOREIGN_FPSTATE to track whether the hardware FPSIMD regs are
>> > > out of sync with the logical state of current's registers.
>> > >
>> > > During sigreturn, the registers and task_struct are temporarily out
>> > > of sync, between writing the task_struct and loading its contents
>> > > back into the FPSIMD registers -- however, TIF_FOREIGN_FPSTATE is
>> > > not set.  This can cause the context switch code to discard some or
>> > > all of the restored FPSIMD state if preemption occurs during the
>> > > critical region of rt_sigreturn.
>> > >
>> > > This patch sets TIF_FOREIGN_FPSTATE before transferring the
>> > > sigframe's saved registers back to the task_struct, so that the
>> > > task_struct data will take precedence over the hardware registers
>> > > if a context switch occurs before everything is back in sync.
>> > >
>> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>> > > [will: removed preempt_{enable,disable} calls, added compat version]
>> > > Signed-off-by: Will Deacon <will.deacon@arm.com>
>> > > ---
>> > >  arch/arm64/kernel/signal.c   | 3 +++
>> > >  arch/arm64/kernel/signal32.c | 2 ++
>> > >  2 files changed, 5 insertions(+)
>> > >
>> > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
>> > > index e18c48cb6db1..6d50d839b6e9 100644
>> > > --- a/arch/arm64/kernel/signal.c
>> > > +++ b/arch/arm64/kernel/signal.c
>> > > @@ -79,6 +79,9 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
>> > >         if (magic != FPSIMD_MAGIC || size != sizeof(struct fpsimd_context))
>> > >                 return -EINVAL;
>> > >
>> > > +       /* Ensure we don't reload stale data from the hardware registers */
>> > > +       set_ti_thread_flag(current_thread_info(), TIF_FOREIGN_FPSTATE);
>> > > +
>> > >         /* copy the FP and status/control registers */
>> > >         err = __copy_from_user(fpsimd.vregs, ctx->vregs,
>> > >                                sizeof(fpsimd.vregs));
>> >
>> > I am not following something here. If the task gets preempted before
>> > the call to fpsimd_update_current_state(), it will proceed to
>> > overwrite the registers with whatever is on the stack in fpsimd after
>> > being scheduled back in, and everything should work fine, right?.
>>
>> You're completely right, sorry for wasting your time. Dave and I have
>> been working on the fpsimd code recently and have another patch that
>> restores the state directly into the current thread to avoid the stack
>> entirely. *That* is what caused this problem, and I incorrectly
>> identified this as a fix for mainline (it's extremely similar to
>> 2af276dfb172 ("ARM: 7306/1: vfp: flush thread hwstate before restoring
>> context from sigframe"), which continues to haunt me).
>
> Urg, looks like I confused myself here.  Ard's assessment looks correct.
>
> Not staging the data via the stack seemed a harmless change which I
> thought was an improvement, since at half a K the amount of data is non-
> trivial.
>
> I'll go back and make some measurements to see whether getting rid
> of the extra copy is really worth it.  I think reading straight into
> the task_struct with __get_user() can work, but it looks like I'll
> need to fiddle with the preemption region, or otherwise handle the
> ensuing race...
>

Indeed. I think you will need something like fpsimd_load_state_user
which runs with preemption disabled and restarts if it is interrupted.

>> Anyway, that also explains why we hadn't seen any problems in our
>> previous testing! The second patch (compat big-endian fix) still stands,
>> however.
>
> I misidentified the problem as being independent of my hacks --
> apologies for the confusion there.
>

No worries.

Ard.
Dave Martin Sept. 23, 2015, 11:05 a.m. | #5
On Tue, Sep 22, 2015 at 06:20:29PM -0700, Ard Biesheuvel wrote:
> On 21 September 2015 at 08:37, Dave P Martin <Dave.Martin@arm.com> wrote:
> > On Tue, Sep 15, 2015 at 03:39:53PM +0100, Will Deacon wrote:
> >> On Tue, Sep 15, 2015 at 01:11:10PM +0100, Ard Biesheuvel wrote:

[...]

> >> > On 15 September 2015 at 13:36, Will Deacon <will.deacon@arm.com> wrote:
> >> > > From: Dave P Martin <Dave.Martin@arm.com>
> >> > >
> >> > > The arm64 context switch implementation uses a flag
> >> > > TIF_FOREIGN_FPSTATE to track whether the hardware FPSIMD regs are
> >> > > out of sync with the logical state of current's registers.
> >> > >
> >> > > During sigreturn, the registers and task_struct are temporarily out
> >> > > of sync, between writing the task_struct and loading its contents
> >> > > back into the FPSIMD registers -- however, TIF_FOREIGN_FPSTATE is
> >> > > not set.  This can cause the context switch code to discard some or
> >> > > all of the restored FPSIMD state if preemption occurs during the
> >> > > critical region of rt_sigreturn.

[...]

> >> > I am not following something here. If the task gets preempted before
> >> > the call to fpsimd_update_current_state(), it will proceed to
> >> > overwrite the registers with whatever is on the stack in fpsimd after
> >> > being scheduled back in, and everything should work fine, right?.

[...]

> > Urg, looks like I confused myself here.  Ard's assessment looks correct.
> >
> > Not staging the data via the stack seemed a harmless change which I
> > thought was an improvement, since at half a K the amount of data is non-
> > trivial.
> >
> > I'll go back and make some measurements to see whether getting rid
> > of the extra copy is really worth it.  I think reading straight into
> > the task_struct with __get_user() can work, but it looks like I'll
> > need to fiddle with the preemption region, or otherwise handle the
> > ensuing race...
> >
> 
> Indeed. I think you will need something like fpsimd_load_state_user
> which runs with preemption disabled and restarts if it is interrupted.

Yep -- I'll take another look at it, and scratch my head some more.

Cheers
---Dave
Ard Biesheuvel Sept. 23, 2015, 2:13 p.m. | #6
On 23 September 2015 at 04:05, Dave Martin <Dave.Martin@arm.com> wrote:
> On Tue, Sep 22, 2015 at 06:20:29PM -0700, Ard Biesheuvel wrote:
>> On 21 September 2015 at 08:37, Dave P Martin <Dave.Martin@arm.com> wrote:
>> > On Tue, Sep 15, 2015 at 03:39:53PM +0100, Will Deacon wrote:
>> >> On Tue, Sep 15, 2015 at 01:11:10PM +0100, Ard Biesheuvel wrote:
>
> [...]
>
>> >> > On 15 September 2015 at 13:36, Will Deacon <will.deacon@arm.com> wrote:
>> >> > > From: Dave P Martin <Dave.Martin@arm.com>
>> >> > >
>> >> > > The arm64 context switch implementation uses a flag
>> >> > > TIF_FOREIGN_FPSTATE to track whether the hardware FPSIMD regs are
>> >> > > out of sync with the logical state of current's registers.
>> >> > >
>> >> > > During sigreturn, the registers and task_struct are temporarily out
>> >> > > of sync, between writing the task_struct and loading its contents
>> >> > > back into the FPSIMD registers -- however, TIF_FOREIGN_FPSTATE is
>> >> > > not set.  This can cause the context switch code to discard some or
>> >> > > all of the restored FPSIMD state if preemption occurs during the
>> >> > > critical region of rt_sigreturn.
>
> [...]
>
>> >> > I am not following something here. If the task gets preempted before
>> >> > the call to fpsimd_update_current_state(), it will proceed to
>> >> > overwrite the registers with whatever is on the stack in fpsimd after
>> >> > being scheduled back in, and everything should work fine, right?.
>
> [...]
>
>> > Urg, looks like I confused myself here.  Ard's assessment looks correct.
>> >
>> > Not staging the data via the stack seemed a harmless change which I
>> > thought was an improvement, since at half a K the amount of data is non-
>> > trivial.
>> >
>> > I'll go back and make some measurements to see whether getting rid
>> > of the extra copy is really worth it.  I think reading straight into
>> > the task_struct with __get_user() can work, but it looks like I'll
>> > need to fiddle with the preemption region, or otherwise handle the
>> > ensuing race...
>> >
>>
>> Indeed. I think you will need something like fpsimd_load_state_user
>> which runs with preemption disabled and restarts if it is interrupted.
>
> Yep -- I'll take another look at it, and scratch my head some more.
>

Actually, I meant 'preemption enabled' (given the __user bit)

Patch

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index e18c48cb6db1..6d50d839b6e9 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -79,6 +79,9 @@  static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 	if (magic != FPSIMD_MAGIC || size != sizeof(struct fpsimd_context))
 		return -EINVAL;
 
+	/* Ensure we don't reload stale data from the hardware registers */
+	set_ti_thread_flag(current_thread_info(), TIF_FOREIGN_FPSTATE);
+
 	/* copy the FP and status/control registers */
 	err = __copy_from_user(fpsimd.vregs, ctx->vregs,
 			       sizeof(fpsimd.vregs));
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index 948f0ad2de23..ae46ffad5aea 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -273,6 +273,8 @@  static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame)
 	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
 		return -EINVAL;
 
+	set_ti_thread_flag(current_thread_info(), TIF_FOREIGN_FPSTATE);
+
 	/*
 	 * Copy the FP registers into the start of the fpsimd_state.
 	 * FIXME: Won't work if big endian.