fix armv8 kernel generation of SIGSEGV upon unaligned access

PATCH Superseded Victor Kamensky 2 participants 5 messages Apr 3, 2017 – Apr 6, 2017 arm64

lore

Share & tools

lore

Victor Kamensky

author

2017-04-03 05:45 UTC #0 lore raw

PATCH 1/1 fix armv8 kernel generation of SIGSEGV upon unaligned access

Hi Will, Catalin,

I discovered that on latest ARMv8 kernel, starting with 4.5, unaligned access by aarch32 ldm instruction (and similar others) produces SIGSEGV to process, which seems to be wrong. On 4.4 kernel the same test produced SIGBUS. The issue was tracked back to 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses)

Following patch proposes the fix. And here are more details and test case that illustrates the issue.

Please note I do not have an idea how to reproduce the issue mentioned by 52d7523. I presume that __do_kernel_fault called by do_bad_area will still take care of it.

Tested system: armv8 kernel and aarch32 rootfs
----------------------------------------------

Note it is ARMv8 kernel and aarch32 rootfs. I thought using unaligned aarch32 ldm is good way to illustrate the issue. My understanding that with aarch64 instructions only unaligned access to device memory will produce unaligned exception, but which is much harder to reproduce.

root at arm-sdk-generic:~/align# uname -a
Linux arm-sdk-generic 4.4.58 #1 SMP PREEMPT Thu Mar 30 21:03:34 PDT 2017 aarch64 aarch64 aarch64 GNU/Linux
root at arm-sdk-generic:~/align# file /lib/systemd/systemd
/lib/systemd/systemd: ELF 32-bit LSB shared object, ARM, EABI5 version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-armhf.so.3, for GNU/Linux 2.6.32, BuildID[sha1]=5c8e03b21f46227d69ad8690ed80ba4ff54b61b3, stripped

Test Code: that produces unaligned access with ldm instructions
---------------------------------------------------------------

root at arm-sdk-generic:~/align# ls
Makefile align.h align1.c main.c
root at arm-sdk-generic:~/align# cat main.c
#include <stdio.h>
#include <signal.h>
#include <unistd.h>
#include "align.h"

static void
sigbus_sa_sigaction(int sig, siginfo_t *context, void *uctx)

{
	printf("%s: got sigbus, sig = %d\n", __FUNCTION__, sig);
	_exit(1);
}

static void
sigsegv_sa_sigaction(int sig, siginfo_t *context, void *uctx)

{
	printf("%s: got sigsegv, should not be here, sig = %d\n", __FUNCTION__, sig);
	_exit(2);
}

void register_signals(void)

{
        struct sigaction sa, sa_old;
	sa.sa_sigaction = sigbus_sa_sigaction;
	sa.sa_flags = SA_SIGINFO;
	sigaction(SIGBUS, &sa, &sa_old);
	
	sa.sa_sigaction = sigsegv_sa_sigaction;
	sigaction(SIGSEGV, &sa, &sa_old);

}

int main (void)

{
	char buf[]="0123456789012345";
	int result;
	register_signals();
	result = load_two((struct twoelem *) (buf + 1));
	printf("result = 0x%x\n", result);
	return 0;
}
root at arm-sdk-generic:~/align# cat align.h 
struct twoelem {
	int i;
	int j;
};

int load_two(struct twoelem *);
root at arm-sdk-generic:~/align# cat align1.c
#include "align.h"

int load_two (struct twoelem *s) {
	return s->i + s->j;
}

root at arm-sdk-generic:~/align# cat Makefile
all: align

CC=gcc
CFLAGS=-O2 -g

align: align1.o main.o

	$(CC) $(CFLAGS) -o $@ $^

clean:
@rm -r align *.o
root at arm-sdk-generic:~/align# make
gcc -O2 -g -c -o align1.o align1.c
gcc -O2 -g -c -o main.o main.c
gcc -O2 -g -o align align1.o main.o
root at arm-sdk-generic:~/align# objdump --disassemble align1.o

align1.o: file format elf32-littlearm

Disassembly of section .text:

00000000 <load_two>:
0: e8900005 ldm r0, {r0, r2}
4: e0820000 add r0, r2, r0
8: e12fff1e bx lr
root at arm-sdk-generic:~/align#

On 4.4 kernel SIGBUS received and unsolicted message from kernel produced
-------------------------------------------------------------------------

That is what we see on our 4.4 kernel: correct signal SIGBUS is invoked. But kernel produces unsolicited message, "Unhandled fault: alignment fault", for every signal: that is what I wanted to fix first.

root at arm-sdk-generic:~/align# uname -a
Linux arm-sdk-generic 4.4.58 #1 SMP PREEMPT Thu Mar 30 21:03:34 PDT 2017 aarch64 aarch64 aarch64 GNU/Linux root at arm-sdk-generic:~/align# ./align
Unhandled fault: alignment fault (0x92000021) at 0x00000000fff6b955
sigbus_sa_sigaction: got sigbus, sig = 7
root at arm-sdk-generic:~/align#

On 4.11-rc4 kernel SIGSEGV received upon the same fault
-------------------------------------------------------

On latest kernel when I've tried the same test I discovered that process now gets SIGSEGV instead of SIGBUS when unaligned ldm instruction is executed. Tracked it down to 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses), which got at kernel starting with 4.5.

root at arm-sdk-generic:~/align# uname -a
Linux arm-sdk-generic 4.11.0-rc4 #1 SMP PREEMPT Thu Mar 30 20:19:56 PDT 2017 aarch64 aarch64 aarch64 GNU/Linux root at arm-sdk-generic:~/align# ./align
sigsegv_sa_sigaction: got sigsegv, should not be here, sig = 11
root at arm-sdk-generic:~/align#

On 4.11-rc4 with my fix, back to SIGBUS
---------------------------------------

After my proposed fix, getting SIGBUS again.

root at arm-sdk-generic:~/align# uname -a
Linux arm-sdk-generic 4.11.0-rc4+ #2 SMP PREEMPT Thu Mar 30 23:53:36 PDT 2017 aarch64 aarch64 aarch64 GNU/Linux root at arm-sdk-generic:~/align# ./align
sigbus_sa_sigaction: got sigbus, sig = 7
root at arm-sdk-generic:~/align#

Thanks,
Victor

Victor Kamensky (1):
arm64: mm: unaligned access by user-land should be received as SIGBUS

 arch/arm64/mm/fault.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Signature · 1 line

1.9.3

Victor Kamensky

author

2017-04-03 05:45 UTC #1 lore raw

PATCH 1/1 arm64: mm: unaligned access by user-land should be received as SIGBUS

After 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses) commit user-land accesses that produce unaligned exceptions like in case of aarch32 ldm/stm/ldrd/strd instructions operating on unaligned memory received by user-land as SIGSEGV. It is wrong, it should be reported as SIGBUS as it was before 52d7523 commit.

Changed do_bad_area function to take signal and code parameters, so caller can pass them down properly depending on fault type, as SIGSEGV in case of do_translation_fault and SIGBUS in case of do_alignment_fault.

Signed-off-by Victor Kamensky <[email protected]>

Cc xe-linux-external at cisco.com

Fixes 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses)

---
 arch/arm64/mm/fault.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

arch/arm64/mm/fault.c +5 -4

index 4bf899f..204eb58 100644
@@ -215,7 +215,8 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
 	force_sig_info(sig, &si, tsk);
 }
 
-static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *regs)
+static void do_bad_area(unsigned long addr, unsigned int esr,
+			struct pt_regs *regs, int sig, int code)
 {
 	struct task_struct *tsk = current;
 	struct mm_struct *mm = tsk->active_mm;

@@ -225,7 +226,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
 	 * handle this fault with.
 	 */
 	if (user_mode(regs))
-		__do_user_fault(tsk, addr, esr, SIGSEGV, SEGV_MAPERR, regs);
+		__do_user_fault(tsk, addr, esr, sig, code, regs);
 	else
 		__do_kernel_fault(mm, addr, esr, regs);
 }

@@ -469,14 +470,14 @@ static int __kprobes do_translation_fault(unsigned long addr,
 	if (addr < TASK_SIZE)
 		return do_page_fault(addr, esr, regs);
 
-	do_bad_area(addr, esr, regs);
+	do_bad_area(addr, esr, regs, SIGSEGV, SEGV_MAPERR);
 	return 0;
 }
 
 static int do_alignment_fault(unsigned long addr, unsigned int esr,
 			      struct pt_regs *regs)
 {
-	do_bad_area(addr, esr, regs);
+	do_bad_area(addr, esr, regs, SIGBUS,  BUS_ADRALN);
 	return 0;
 }

Signature · 1 line

1.9.3

Will Deacon

2017-04-03 09:24 UTC #2 lore raw

PATCH 1/1 arm64: mm: unaligned access by user-land should be received as SIGBUS

Hi Victor,

Thanks for reporting this.

Quoted 44 lines · Victor Kamensky

After 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses) commit user-land accesses that produce unaligned exceptions like in case of aarch32 ldm/stm/ldrd/strd instructions operating on unaligned memory received by user-land as SIGSEGV. It is wrong, it should be reported as SIGBUS as it was before 52d7523 commit.
Changed do_bad_area function to take signal and code parameters, so caller can pass them down properly depending on fault type, as SIGSEGV in case of do_translation_fault and SIGBUS in case of do_alignment_fault.
Signed-off-by Victor Kamensky <[email protected]>
Cc xe-linux-external at cisco.com
Fixes 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses)
---
 arch/arm64/mm/fault.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
arch/arm64/mm/fault.c +3 -2
index 4bf899f..204eb58 100644
@@ -215,7 +215,8 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
 	force_sig_info(sig, &si, tsk);
 }
 
-static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *regs)
+static void do_bad_area(unsigned long addr, unsigned int esr,
+			struct pt_regs *regs, int sig, int code)
 {
 	struct task_struct *tsk = current;
 	struct mm_struct *mm = tsk->active_mm;
@@ -225,7 +226,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
 	 * handle this fault with.
 	 */
 	if (user_mode(regs))
-		__do_user_fault(tsk, addr, esr, SIGSEGV, SEGV_MAPERR, regs);
+		__do_user_fault(tsk, addr, esr, sig, code, regs);
 	else
 		__do_kernel_fault(mm, addr, esr, regs);
 }
@@ -469,14 +470,14 @@ static int __kprobes do_translation_fault(unsigned long addr,
 	if (addr < TASK_SIZE)
 		return do_page_fault(addr, esr, regs);
 

arch/arm64/mm/fault.c static int __kprobes do_translation_fault(unsigned long addr, · line 480

Quoted 10 lines

arch/arm64/mm/fault.c +2 -2

-	do_bad_area(addr, esr, regs);
+	do_bad_area(addr, esr, regs, SIGSEGV, SEGV_MAPERR);
 	return 0;
 }
 
 static int do_alignment_fault(unsigned long addr, unsigned int esr,
 			      struct pt_regs *regs)
 {
-	do_bad_area(addr, esr, regs);
+	do_bad_area(addr, esr, regs, SIGBUS,  BUS_ADRALN);

Can we not just extract the signal number and code from the fault info table?

E.g. leave the type signature of do_bad_area like it is, but do:

	const struct fault_info *inf = fault_info + (esr & 63);
	__do_user_fault(tsk, addr, esr, inf->sig, inf->code, regs);

The '& 63' is ugly as hell, so maybe wrap that up in a esr_to_fault_info function and kill the fault_name thing we have at the moment.

Will

Victor Kamensky

author

2017-04-06 06:01 UTC #3 lore raw

PATCH 1/1 arm64: mm: unaligned access by user-land should be received as SIGBUS

Hi Will,

Quoted 63 lines · Will Deacon

Hi Victor,
Thanks for reporting this.
Quoted 44 lines · Victor Kamensky
After 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses) commit user-land accesses that produce unaligned exceptions like in case of aarch32 ldm/stm/ldrd/strd instructions operating on unaligned memory received by user-land as SIGSEGV. It is wrong, it should be reported as SIGBUS as it was before 52d7523 commit.
Changed do_bad_area function to take signal and code parameters, so caller can pass them down properly depending on fault type, as SIGSEGV in case of do_translation_fault and SIGBUS in case of do_alignment_fault.
Signed-off-by Victor Kamensky <[email protected]>
Cc xe-linux-external at cisco.com
Fixes 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses)
---
 arch/arm64/mm/fault.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
arch/arm64/mm/fault.c +3 -2
index 4bf899f..204eb58 100644
@@ -215,7 +215,8 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
 	force_sig_info(sig, &si, tsk);
 }
 
-static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *regs)
+static void do_bad_area(unsigned long addr, unsigned int esr,
+			struct pt_regs *regs, int sig, int code)
 {
 	struct task_struct *tsk = current;
 	struct mm_struct *mm = tsk->active_mm;
@@ -225,7 +226,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
 	 * handle this fault with.
 	 */
 	if (user_mode(regs))
-		__do_user_fault(tsk, addr, esr, SIGSEGV, SEGV_MAPERR, regs);
+		__do_user_fault(tsk, addr, esr, sig, code, regs);
 	else
 		__do_kernel_fault(mm, addr, esr, regs);
 }
@@ -469,14 +470,14 @@ static int __kprobes do_translation_fault(unsigned long addr,
 	if (addr < TASK_SIZE)
 		return do_page_fault(addr, esr, regs);
 
arch/arm64/mm/fault.c static int __kprobes do_translation_fault(unsigned long addr, · line 480
Quoted 10 lines
arch/arm64/mm/fault.c +2 -2
-	do_bad_area(addr, esr, regs);
+	do_bad_area(addr, esr, regs, SIGSEGV, SEGV_MAPERR);
 	return 0;
 }
 
 static int do_alignment_fault(unsigned long addr, unsigned int esr,
 			      struct pt_regs *regs)
 {
-	do_bad_area(addr, esr, regs);
+	do_bad_area(addr, esr, regs, SIGBUS,  BUS_ADRALN);
Can we not just extract the signal number and code from the fault info table?

Quoted 7 lines

E.g. leave the type signature of do_bad_area like it is, but do:
	const struct fault_info *inf = fault_info + (esr & 63);
	__do_user_fault(tsk, addr, esr, inf->sig, inf->code, regs);
The '& 63' is ugly as hell, so maybe wrap that up in a esr_to_fault_info function and kill the fault_name thing we have at the moment.

Could you please take a look at [1]. I've tried to reimplement the fix following your suggestion.

Initially I wanted to fix do_bad_area function with use of fault_info array as you suggested, but realized that fix would be more involved, since struct fault_info and array fault_info defined together and one would need to move either do_bad_area function below fault_info array definition or move struct fault_info above it. In my [1] fix I choose the latter.

[1] http://archive.armlinux.org.uk/lurker/message/20170404.055101.e6d54d8e.en.html

Thanks,
Victor

Quoted 2 lines

Will

Will Deacon

2017-04-06 08:44 UTC #4 lore raw

PATCH 1/1 arm64: mm: unaligned access by user-land should be received as SIGBUS

Hi Victor,

Quoted 62 lines · Victor Kamensky

Quoted 61 lines · Will Deacon
Quoted 44 lines · Victor Kamensky
After 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses) commit user-land accesses that produce unaligned exceptions like in case of aarch32 ldm/stm/ldrd/strd instructions operating on unaligned memory received by user-land as SIGSEGV. It is wrong, it should be reported as SIGBUS as it was before 52d7523 commit.
Changed do_bad_area function to take signal and code parameters, so caller can pass them down properly depending on fault type, as SIGSEGV in case of do_translation_fault and SIGBUS in case of do_alignment_fault.
Signed-off-by Victor Kamensky <[email protected]>
Cc xe-linux-external at cisco.com
Fixes 52d7523 (arm64: mm: allow the kernel to handle alignment faults on user accesses)
---
arch/arm64/mm/fault.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
arch/arm64/mm/fault.c +3 -2
index 4bf899f..204eb58 100644
@@ -215,7 +215,8 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr,
 	force_sig_info(sig, &si, tsk);
 }
 
-static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *regs)
+static void do_bad_area(unsigned long addr, unsigned int esr,
+			struct pt_regs *regs, int sig, int code)
 {
 	struct task_struct *tsk = current;
 	struct mm_struct *mm = tsk->active_mm;
@@ -225,7 +226,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
 	 * handle this fault with.
 	 */
 	if (user_mode(regs))
-		__do_user_fault(tsk, addr, esr, SIGSEGV, SEGV_MAPERR, regs);
+		__do_user_fault(tsk, addr, esr, sig, code, regs);
 	else
 		__do_kernel_fault(mm, addr, esr, regs);
 }
@@ -469,14 +470,14 @@ static int __kprobes do_translation_fault(unsigned long addr,
 	if (addr < TASK_SIZE)
 		return do_page_fault(addr, esr, regs);
 
arch/arm64/mm/fault.c static int __kprobes do_translation_fault(unsigned long addr, · line 480
Quoted 10 lines
arch/arm64/mm/fault.c +2 -2
-	do_bad_area(addr, esr, regs);
+	do_bad_area(addr, esr, regs, SIGSEGV, SEGV_MAPERR);
 	return 0;
 }
 
 static int do_alignment_fault(unsigned long addr, unsigned int esr,
 			      struct pt_regs *regs)
 {
-	do_bad_area(addr, esr, regs);
+	do_bad_area(addr, esr, regs, SIGBUS,  BUS_ADRALN);
Can we not just extract the signal number and code from the fault info table?
E.g. leave the type signature of do_bad_area like it is, but do:

Quoted 8 lines

Quoted 5 lines
	const struct fault_info *inf = fault_info + (esr & 63);
	__do_user_fault(tsk, addr, esr, inf->sig, inf->code, regs);
The '& 63' is ugly as hell, so maybe wrap that up in a esr_to_fault_info function and kill the fault_name thing we have at the moment.
Could you please take a look at [1]. I've tried to reimplement the fix following your suggestion.

Yup, I picked this up here already:

https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=fixes/core

Thanks,

Will