Unaligned access in kernel on ARMv6+ (Re: CVS commit: src/sys/dev/usb)

[Rin Okuyama <rokuyama%rk.phys.keio.ac.jp@localhost>]

(CC added to port-arm%NetBSD.org@localhost)

Let me summarize the problem briefly. In axe(4), there is a code where
memcpy() is carried out from 2-byte aligned buffer to 4-byte structure:

https://nxr.netbsd.org/xref/src/sys/dev/usb/if_axe.c#1284

This results in kernel panic due to alignment fault on earmv[67]hf:

https://twitter.com/furandon_pig/status/1071771151418908672

In short, this is because -munaligned-access is enabled on ARMv6+ by
default for GCC. As the unaligned memory access is forbidden in the
supervisor mode unlike in the user mode, we need to explicitly specify
-mno-unaligned-access for kernel on ARMv6+.

On 2019/01/06 12:59, matthew green wrote:
> Christos Zoulas writes:
> > In article <20190106003905.60969FB17%cvs.NetBSD.org@localhost>,
> > Rin Okuyama <source-changes-d%NetBSD.org@localhost> wrote:
> > > -=-=-=-=-=-
> > >
> > > Module Name:	src
> > > Committed By:	rin
> > > Date:		Sun Jan  6 00:39:05 UTC 2019
> > >
> > > Modified Files:
> > > 	src/sys/dev/usb: if_axe.c
> > >
> > > Log Message:
> > > Fix kernel panic on arm reported by @furandon_pig on Twitter.
> > >
> > > Hardware header is 2-byte aligned in RX buffer, not 4-byte.
> > > For some architectures, __builtin_memcpy() of GCC 6 attempts to
> > > copy 4-byte header at once, which results in alignment error.
> >
> > This is really ugly..
> >
> > https://stackoverflow.com/questions/24883410/armcc-problems-with-memcpy-alignment-exceptions
> >
> > Perhaps there is a better solution? Can't memcpy be smarter?
>
> hmmm, what happens if struct axe_sframe_hdr is not marked
> "packed"?  this feels like a compiler bug, but perhaps it
> is assuming it can write 4 bytes to the structure when it
> is only 2 byte aligned.
>
> is there a small test case that reproduces the problem?
> preferably in user land?

On 2019/01/06 15:25, maya%netbsd.org@localhost wrote:
> Are we building ARM with -mstrict-alignment?

I tried to reproduce the problem on userland. objdump(1) shows an
unaligned load is generated. However, alignment fault does not occur:

% uname -p
earmv7hf
% cat test.c
#include <stdio.h>
#include <string.h>

int
main()
{
        char buf[sizeof(int) + 1];
        int i;

        fread(buf, 1, sizeof(buf), stdin);
        memcpy(&i, &buf[1], sizeof(i));
        printf("0x%x\n", i);
        return 0;
}
% cc -g -O2 test.c && cc test.o
% objdump test.o
...
  28:   e51b1013        ldr     r1, [fp, #-19]  ; 0xffffffed
...
% ./a.out
01234
0x34333231

This is because unaligned access is permitted for the user mode on
ARMv6+. For GCC, -munaligned-access is enabled by default on ARMv6+.
However, the unaligned access is forbidden in the supervisor mode.
So, we need to explicitly specify -mno-unaligned-access for kernel
on ARMv6+.

By reverting if_axe.c r1.94 and applying the attached patch, axe(4)
works fine on earmv7hf. We can see that the instruction is changed
from word-wise load to byte-wise load by specifying
-mno-unaligned-access:

% armv7--netbsdelf-eabihf-objdump -d if_axe.o
(before)     364:       e4983004        ldr     r3, [r8], #4
--->
(after)      364:       e5d60000        ldrb    r0, [r6]

I guess other codes can be miscompiled if -mno-unaligned-access is
not specified. Can I commit the patch?

Thanks,
rin
----
Index: sys/arch/arm/conf/Makefile.arm
===================================================================
RCS file: /home/netbsd/src/sys/arch/arm/conf/Makefile.arm,v
retrieving revision 1.49
diff -p -u -r1.49 Makefile.arm
--- sys/arch/arm/conf/Makefile.arm	22 Sep 2018 12:24:01 -0000	1.49
+++ sys/arch/arm/conf/Makefile.arm	6 Jan 2019 08:14:56 -0000
@@ -53,6 +53,13 @@ CPPFLAGS.cpufunc_asm_armv6.S+=	-mcpu=arm
 CPPFLAGS.cpufunc_asm_arm11.S+=	-mcpu=arm1136j-s
 CPPFLAGS.cpufunc_asm_xscale.S+=	-mcpu=xscale
 
+# For GCC, -munaligned-access is enabled by default for ARMv6+.
+# But the unaligned access is forbidden in the supervisor mode.
+.if (!empty(MACHINE_ARCH:Mearmv6*) || !empty(MACHINE_ARCH:Mearmv7*)) \
+    && ${ACTIVE_CC} == "gcc"
+CFLAGS+=	-mno-unaligned-access
+.endif
+
 ##
 ## (3) libkern and compat
 ##