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

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

[Will Deacon]

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));
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.
-- 
2.1.4

[PATCH 2/2] arm64: compat: fix vfp save/restore across signal handlers in big-endian

[Will Deacon]

When saving/restoring the VFP registers from a compat (AArch32)
signal frame, we rely on the compat registers forming a prefix of the
native register file and therefore make use of copy_{to,from}_user to
transfer between the native fpsimd_state and the compat_vfp_sigframe.

Unfortunately, this doesn't work so well in a big-endian environment.
Our fpsimd save/restore code operates directly on 128-bit quantities
(Q registers) whereas the compat_vfp_sigframe represents the registers
as an array of 64-bit (D) registers. The architecture packs the compat D
registers into the Q registers, with the least significant bytes holding
the lower register. Consequently, we need to swap the 64-bit halves when
converting between these two representations on a big-endian machine.

This patch replaces the __copy_{to,from}_user invocations in our
compat VFP signal handling code with explicit __put_user loops that
operate on 64-bit values and swap them accordingly.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/signal32.c | 47 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index ae46ffad5aea..64e498e59f99 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -212,14 +212,32 @@ int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
 
 /*
  * VFP save/restore code.
+ *
+ * We have to be careful with endianness, since the fpsimd context-switch
+ * code operates on 128-bit (Q) register values whereas the compat ABI
+ * uses an array of 64-bit (D) registers. Consequently, we need to swap
+ * the two halves of each Q register when running on a big-endian CPU.
  */
+union __fpsimd_vreg {
+	__uint128_t	raw;
+	struct {
+#ifdef __AARCH64EB__
+		u64	hi;
+		u64	lo;
+#else
+		u64	lo;
+		u64	hi;
+#endif
+	};
+};
+
 static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
 {
 	struct fpsimd_state *fpsimd = &current->thread.fpsimd_state;
 	compat_ulong_t magic = VFP_MAGIC;
 	compat_ulong_t size = VFP_STORAGE_SIZE;
 	compat_ulong_t fpscr, fpexc;
-	int err = 0;
+	int i, err = 0;
 
 	/*
 	 * Save the hardware registers to the fpsimd_state structure.
@@ -235,10 +253,15 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
 	/*
 	 * Now copy the FP registers. Since the registers are packed,
 	 * we can copy the prefix we want (V0-V15) as it is.
-	 * FIXME: Won't work if big endian.
 	 */
-	err |= __copy_to_user(&frame->ufp.fpregs, fpsimd->vregs,
-			      sizeof(frame->ufp.fpregs));
+	for (i = 0; i < ARRAY_SIZE(frame->ufp.fpregs); i++) {
+		union __fpsimd_vreg vreg = {
+			.raw = fpsimd->vregs[i >> 1],
+		};
+
+		__put_user_error(vreg.lo, &frame->ufp.fpregs[i++], err);
+		__put_user_error(vreg.hi, &frame->ufp.fpregs[i], err);
+	}
 
 	/* Create an AArch32 fpscr from the fpsr and the fpcr. */
 	fpscr = (fpsimd->fpsr & VFP_FPSCR_STAT_MASK) |
@@ -263,7 +286,7 @@ static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame)
 	compat_ulong_t magic = VFP_MAGIC;
 	compat_ulong_t size = VFP_STORAGE_SIZE;
 	compat_ulong_t fpscr;
-	int err = 0;
+	int i, err = 0;
 
 	__get_user_error(magic, &frame->magic, err);
 	__get_user_error(size, &frame->size, err);
@@ -275,12 +298,14 @@ static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame)
 
 	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.
-	 */
-	err |= __copy_from_user(fpsimd.vregs, frame->ufp.fpregs,
-				sizeof(frame->ufp.fpregs));
+	/* Copy the FP registers into the start of the fpsimd_state. */
+	for (i = 0; i < ARRAY_SIZE(frame->ufp.fpregs); i++) {
+		union __fpsimd_vreg vreg;
+
+		__get_user_error(vreg.lo, &frame->ufp.fpregs[i++], err);
+		__get_user_error(vreg.hi, &frame->ufp.fpregs[i], err);
+		fpsimd.vregs[i >> 1] = vreg.raw;
+	}
 
 	/* Extract the fpsr and the fpcr from the fpscr */
 	__get_user_error(fpscr, &frame->ufp.fpscr, err);
-- 
2.1.4

[PATCH 2/2] arm64: compat: fix vfp save/restore across signal handlers in big-endian

[Catalin Marinas]

On Tue, Sep 15, 2015 at 12:36:37PM +0100, Will Deacon wrote:
> @@ -235,10 +253,15 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
>  	/*
>  	 * Now copy the FP registers. Since the registers are packed,
>  	 * we can copy the prefix we want (V0-V15) as it is.
> -	 * FIXME: Won't work if big endian.
>  	 */
> -	err |= __copy_to_user(&frame->ufp.fpregs, fpsimd->vregs,
> -			      sizeof(frame->ufp.fpregs));
> +	for (i = 0; i < ARRAY_SIZE(frame->ufp.fpregs); i++) {
> +		union __fpsimd_vreg vreg = {
> +			.raw = fpsimd->vregs[i >> 1],
> +		};
> +
> +		__put_user_error(vreg.lo, &frame->ufp.fpregs[i++], err);
> +		__put_user_error(vreg.hi, &frame->ufp.fpregs[i], err);
> +	}

Nitpick: I find it easier to read as (same for the other hunk):

	for (i = 0; i < ARRAY_SIZE(frame->ufp.fpregs); i += 2) {
		...

		__put_user_error(vreg.lo, &frame->ufp.fpregs[i], err);
		__put_user_error(vreg.hi, &frame->ufp.fpregs[i + 1], err);
	}

It's up to you. Anyway:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

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

[Ard Biesheuvel]

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)

-- 
Ard.




> 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.
> --
> 2.1.4
>

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

[Will Deacon]

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

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

[Dave P Martin]

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

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

[Ard Biesheuvel]

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.

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

[Dave Martin]

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

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

[Ard Biesheuvel]

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)