[PATCH] ARM: plug a race with the alignment trap handler

[PATCH] ARM: plug a race with the alignment trap handler

[Nicolas Pitre]

When the policy for user space is to ignore misaligned accesses from user
space, the processor then performs a documented rotation on the accessed
data.  This is the result of the access being trapped, and the kernel
disabling the alignment trap before returning to user space again.

In kernel space we always want misaligned accesses to be fixed up.  This
is enforced by always re-enabling the alignment trap on every entry into
kernel space from user space.  No such re-enabling is performed when an
exception occurs while already in kernel space as the alignment trap is
always supposed to be enabled in that case.

There is however a small race window when a misaligned access in user
space is trapped and the alignment trap disabled, but the CPU didn't
return to user space just yet.  Any exception would be entered from kernel
space at that point and the kernel would then execute with the alignment
trap disabled.

Thanks to Maxime Bizon <mbizon@freebox.fr> for providing a test module
that made this issue reproducible.

Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>

diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index d073b64..724ba3b 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -885,8 +885,23 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 
 	if (ai_usermode & UM_SIGNAL)
 		force_sig(SIGBUS, current);
-	else
-		set_cr(cr_no_alignment);
+	else {
+		/*
+		 * We're about to disable the alignment trap and return to
+		 * user space.  But if an interrupt occurs before actually
+		 * reaching user space, then the IRQ vector entry code will
+		 * notice that we were still in kernel space and therefore
+		 * the alignment trap won't be re-enabled in that case as it
+		 * is presumed to be always on from kernel space.
+		 * Let's prevent that race by disabling interrupts here (they
+		 * are disabled on the way back to user space anyway in
+		 * entry-common.S) and disable the alignment trap only if
+		 * there is no work pending for this thread.
+		 */
+		raw_local_irq_disable();
+		if (!(current_thread_info()->flags & _TIF_WORK_MASK))
+			set_cr(cr_no_alignment);
+	}
 
 	return 0;
 }

[PATCH] ARM: plug a race with the alignment trap handler

[Russell King - ARM Linux]

On Tue, Sep 14, 2010 at 11:35:27PM -0400, Nicolas Pitre wrote:
> There is however a small race window when a misaligned access in user
> space is trapped and the alignment trap disabled, but the CPU didn't
> return to user space just yet.  Any exception would be entered from kernel
> space at that point and the kernel would then execute with the alignment
> trap disabled.

This isn't good enough - you can't just disable interrupts and hope
that they'll remain that way.

Consider what happens if the threads time slice has expired, and
TIF_NEED_RESCHED is set - the result will be that we call schedule()
and possibly switch to another thread with alignment faults disabled.

I keep on toying with an idea to use prctl() for alignment faults,
and whether to revamp this code to interact with that - which means
programs can on an individual basis decide how they want alignment
faults to be dealt with.

This would mean storing a per-thread copy of the control register,
which means that the entry*.S code can deal with updating the A bit.

[PATCH] ARM: plug a race with the alignment trap handler

[Nicolas Pitre]

On Mon, 20 Sep 2010, Russell King - ARM Linux wrote:

> On Tue, Sep 14, 2010 at 11:35:27PM -0400, Nicolas Pitre wrote:
> > There is however a small race window when a misaligned access in user
> > space is trapped and the alignment trap disabled, but the CPU didn't
> > return to user space just yet.  Any exception would be entered from kernel
> > space at that point and the kernel would then execute with the alignment
> > trap disabled.
> 
> This isn't good enough - you can't just disable interrupts and hope
> that they'll remain that way.
> 
> Consider what happens if the threads time slice has expired, and
> TIF_NEED_RESCHED is set - the result will be that we call schedule()
> and possibly switch to another thread with alignment faults disabled.

No. This is covered in my patch, which is essentially:

	raw_local_irq_disable();
	if (!(current_thread_info()->flags & _TIF_WORK_MASK))
		set_cr(cr_no_alignment);

So if we are going to schedule or whatever, then the alignment trap 
remains active, and the user misaligned access will trap again later.

> I keep on toying with an idea to use prctl() for alignment faults,
> and whether to revamp this code to interact with that - which means
> programs can on an individual basis decide how they want alignment
> faults to be dealt with.

Altough this looks nice, I doubt this would be useful in practice.  
Most of the universe is expecting misaligned accesses to be fixed up.  
So if the fixup has to be enabled on a per application basis that would 
make things really awkward, and a real PITA if that has to be 
accomplished in the form of source code modification.  That goes without 
saying that nothing compiled for EABI expects the non fixed up 
misaligned access behavior.

If, instead, the default is to have the fixup active by default and that 
applications that doesn't want it would have to disable it explicitly 
then I agree that this would be more useful.  However my understanding 
is that the only case where disabling the fixup is needed is for legacy 
binaries and that may prove difficult to recompile them to add this 
prctl() call.


Nicolas