From 526c3ddb6aa270fe6f71d227eac1e189746cc257 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Wed, 3 Jan 2018 18:07:12 -0600 Subject: signal/arm64: Document conflicts with SI_USER and SIGFPE,SIGTRAP,SIGBUS Setting si_code to 0 results in a userspace seeing an si_code of 0. This is the same si_code as SI_USER. Posix and common sense requires that SI_USER not be a signal specific si_code. As such this use of 0 for the si_code is a pretty horribly broken ABI. Further use of si_code == 0 guaranteed that copy_siginfo_to_user saw a value of __SI_KILL and now sees a value of SIL_KILL with the result that uid and pid fields are copied and which might copying the si_addr field by accident but certainly not by design. Making this a very flakey implementation. Utilizing FPE_FIXME, BUS_FIXME, TRAP_FIXME siginfo_layout will now return SIL_FAULT and the appropriate fields will be reliably copied. But folks this is a new and unique kind of bad. This is massively untested code bad. This is inventing new and unique was to get siginfo wrong bad. This is don't even think about Posix or what siginfo means bad. This is lots of eyeballs all missing the fact that the code does the wrong thing bad. This is getting stuck and keep making the same mistake bad. I really hope we can find a non userspace breaking fix for this on a port as new as arm64. Possible ABI fixes include: - Send the signal without siginfo - Don't generate a signal - Possibly assign and use an appropriate si_code - Don't handle cases which can't happen Cc: Catalin Marinas Cc: Will Deacon Cc: Tyler Baicar Cc: James Morse Cc: Tony Lindgren Cc: Nicolas Pitre Cc: Olof Johansson Cc: Santosh Shilimkar Cc: Arnd Bergmann Cc: linux-arm-kernel@lists.infradead.org Ref: 53631b54c870 ("arm64: Floating point and SIMD") Ref: 32015c235603 ("arm64: exception: handle Synchronous External Abort") Ref: 1d18c47c735e ("arm64: MMU fault handling and page table management") Signed-off-by: "Eric W. Biederman" --- arch/arm64/kernel/fpsimd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'arch/arm64/kernel') diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index fae81f7964b4..ad0edf31e247 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -867,7 +867,7 @@ asmlinkage void do_fpsimd_acc(unsigned int esr, struct pt_regs *regs) asmlinkage void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs) { siginfo_t info; - unsigned int si_code = 0; + unsigned int si_code = FPE_FIXME; if (esr & FPEXC_IOF) si_code = FPE_FLTINV; -- cgit v1.2.3 From 212a36a17efe4d696d1e3c31ebd79a9fb0cbb14b Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 31 Jul 2017 17:15:31 -0500 Subject: signal: Unify and correct copy_siginfo_from_user32 The function copy_siginfo_from_user32 is used for two things, in ptrace since the dawn of siginfo for arbirarily modifying a signal that user space sees, and in sigqueueinfo to send a signal with arbirary siginfo data. Create a single copy of copy_siginfo_from_user32 that all architectures share, and teach it to handle all of the cases in the siginfo union. In the generic version of copy_siginfo_from_user32 ensure that all of the fields in siginfo are initialized so that the siginfo structure can be safely copied to userspace if necessary. When copying the embedded sigval union copy the si_int member. That ensures the 32bit values passes through the kernel unchanged. Signed-off-by: "Eric W. Biederman" --- arch/arm64/kernel/signal32.c | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'arch/arm64/kernel') diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c index 22711ee8e36c..4377907dbb70 100644 --- a/arch/arm64/kernel/signal32.c +++ b/arch/arm64/kernel/signal32.c @@ -195,16 +195,6 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from) return err; } -int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from) -{ - if (copy_from_user(to, from, __ARCH_SI_PREAMBLE_SIZE) || - copy_from_user(to->_sifields._pad, - from->_sifields._pad, SI_PAD_SIZE)) - return -EFAULT; - - return 0; -} - /* * VFP save/restore code. * -- cgit v1.2.3 From ea64d5acc8f033cd586182ae31531246cdeaea73 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 15 Jan 2018 18:03:33 -0600 Subject: signal: Unify and correct copy_siginfo_to_user32 Among the existing architecture specific versions of copy_siginfo_to_user32 there are several different implementation problems. Some architectures fail to handle all of the cases in in the siginfo union. Some architectures perform a blind copy of the siginfo union when the si_code is negative. A blind copy suggests the data is expected to be in 32bit siginfo format, which means that receiving such a signal via signalfd won't work, or that the data is in 64bit siginfo and the code is copying nonsense to userspace. Create a single instance of copy_siginfo_to_user32 that all of the architectures can share, and teach it to handle all of the cases in the siginfo union correctly, with the assumption that siginfo is stored internally to the kernel is 64bit siginfo format. A special case is made for x86 x32 format. This is needed as presence of both x32 and ia32 on x86_64 results in two different 32bit signal formats. By allowing this small special case there winds up being exactly one code base that needs to be maintained between all of the architectures. Vastly increasing the testing base and the chances of finding bugs. As the x86 copy of copy_siginfo_to_user32 the call of the x86 signal_compat_build_tests were moved into sigaction_compat_abi, so that they will keep running. Signed-off-by: "Eric W. Biederman" --- arch/arm64/kernel/signal32.c | 70 -------------------------------------------- 1 file changed, 70 deletions(-) (limited to 'arch/arm64/kernel') diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c index 4377907dbb70..cbc4edd1b1eb 100644 --- a/arch/arm64/kernel/signal32.c +++ b/arch/arm64/kernel/signal32.c @@ -125,76 +125,6 @@ static inline int get_sigset_t(sigset_t *set, return 0; } -int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from) -{ - int err; - - if (!access_ok(VERIFY_WRITE, to, sizeof(*to))) - return -EFAULT; - - /* If you change siginfo_t structure, please be sure - * this code is fixed accordingly. - * It should never copy any pad contained in the structure - * to avoid security leaks, but must copy the generic - * 3 ints plus the relevant union member. - * This routine must convert siginfo from 64bit to 32bit as well - * at the same time. - */ - err = __put_user(from->si_signo, &to->si_signo); - err |= __put_user(from->si_errno, &to->si_errno); - err |= __put_user(from->si_code, &to->si_code); - if (from->si_code < 0) - err |= __copy_to_user(&to->_sifields._pad, &from->_sifields._pad, - SI_PAD_SIZE); - else switch (siginfo_layout(from->si_signo, from->si_code)) { - case SIL_KILL: - err |= __put_user(from->si_pid, &to->si_pid); - err |= __put_user(from->si_uid, &to->si_uid); - break; - case SIL_TIMER: - err |= __put_user(from->si_tid, &to->si_tid); - err |= __put_user(from->si_overrun, &to->si_overrun); - err |= __put_user(from->si_int, &to->si_int); - break; - case SIL_POLL: - err |= __put_user(from->si_band, &to->si_band); - err |= __put_user(from->si_fd, &to->si_fd); - break; - case SIL_FAULT: - err |= __put_user((compat_uptr_t)(unsigned long)from->si_addr, - &to->si_addr); -#ifdef BUS_MCEERR_AO - /* - * Other callers might not initialize the si_lsb field, - * so check explicitly for the right codes here. - */ - if (from->si_signo == SIGBUS && - (from->si_code == BUS_MCEERR_AR || from->si_code == BUS_MCEERR_AO)) - err |= __put_user(from->si_addr_lsb, &to->si_addr_lsb); -#endif - break; - case SIL_CHLD: - err |= __put_user(from->si_pid, &to->si_pid); - err |= __put_user(from->si_uid, &to->si_uid); - err |= __put_user(from->si_status, &to->si_status); - err |= __put_user(from->si_utime, &to->si_utime); - err |= __put_user(from->si_stime, &to->si_stime); - break; - case SIL_RT: - err |= __put_user(from->si_pid, &to->si_pid); - err |= __put_user(from->si_uid, &to->si_uid); - err |= __put_user(from->si_int, &to->si_int); - break; - case SIL_SYS: - err |= __put_user((compat_uptr_t)(unsigned long) - from->si_call_addr, &to->si_call_addr); - err |= __put_user(from->si_syscall, &to->si_syscall); - err |= __put_user(from->si_arch, &to->si_arch); - break; - } - return err; -} - /* * VFP save/restore code. * -- cgit v1.2.3 From 66e0f26315ce7dd3f4efdbdee63f30dac643763f Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 22 Jan 2018 14:50:53 -0600 Subject: signal/arm64: Better isolate the COMPAT_TASK portion of ptrace_hbptriggered Instead of jumpping while !is_compat_task placee all of the code inside of an if (is_compat_task) block. This allows the int i variable to be properly limited to the compat block no matter how the rest of ptrace_hbptriggered changes. In a following change a non-variable declaration will preceed was made independent to ensure the code is easy to review. Signed-off-by: "Eric W. Biederman" --- arch/arm64/kernel/ptrace.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) (limited to 'arch/arm64/kernel') diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 7c44658b316d..0a1cf830e4b3 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -188,26 +188,23 @@ static void ptrace_hbptriggered(struct perf_event *bp, }; #ifdef CONFIG_COMPAT - int i; - - if (!is_compat_task()) - goto send_sig; + if (is_compat_task()) { + int i; - for (i = 0; i < ARM_MAX_BRP; ++i) { - if (current->thread.debug.hbp_break[i] == bp) { - info.si_errno = (i << 1) + 1; - break; + for (i = 0; i < ARM_MAX_BRP; ++i) { + if (current->thread.debug.hbp_break[i] == bp) { + info.si_errno = (i << 1) + 1; + break; + } } - } - for (i = 0; i < ARM_MAX_WRP; ++i) { - if (current->thread.debug.hbp_watch[i] == bp) { - info.si_errno = -((i << 1) + 1); - break; + for (i = 0; i < ARM_MAX_WRP; ++i) { + if (current->thread.debug.hbp_watch[i] == bp) { + info.si_errno = -((i << 1) + 1); + break; + } } } - -send_sig: #endif force_sig_info(SIGTRAP, &info, current); } -- cgit v1.2.3 From 5f74972ce69fdc6473f74253283408af75a3be15 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 22 Jan 2018 14:58:57 -0600 Subject: signal: Don't use structure initializers for struct siginfo The siginfo structure has all manners of holes with the result that a structure initializer is not guaranteed to initialize all of the bits. As we have to copy the structure to userspace don't even try to use a structure initializer. Instead use clear_siginfo followed by initializing selected fields. This gives a guarantee that uninitialized kernel memory is not copied to userspace. Signed-off-by: "Eric W. Biederman" --- arch/arm64/kernel/debug-monitors.c | 13 +++++++------ arch/arm64/kernel/ptrace.c | 13 +++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) (limited to 'arch/arm64/kernel') diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index a88b6ccebbb4..53781f5687c5 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -209,12 +209,13 @@ NOKPROBE_SYMBOL(call_step_hook); static void send_user_sigtrap(int si_code) { struct pt_regs *regs = current_pt_regs(); - siginfo_t info = { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = si_code, - .si_addr = (void __user *)instruction_pointer(regs), - }; + siginfo_t info; + + clear_siginfo(&info); + info.si_signo = SIGTRAP; + info.si_errno = 0; + info.si_code = si_code; + info.si_addr = (void __user *)instruction_pointer(regs); if (WARN_ON(!user_mode(regs))) return; diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 0a1cf830e4b3..95daa1478a7c 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -180,12 +180,13 @@ static void ptrace_hbptriggered(struct perf_event *bp, struct pt_regs *regs) { struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp); - siginfo_t info = { - .si_signo = SIGTRAP, - .si_errno = 0, - .si_code = TRAP_HWBKPT, - .si_addr = (void __user *)(bkpt->trigger), - }; + siginfo_t info; + + clear_siginfo(&info); + info.si_signo = SIGTRAP; + info.si_errno = 0; + info.si_code = TRAP_HWBKPT; + info.si_addr = (void __user *)(bkpt->trigger); #ifdef CONFIG_COMPAT if (is_compat_task()) { -- cgit v1.2.3 From f71dd7dc2dc989dc712b246a74d243e4b2c5f8a7 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 22 Jan 2018 14:37:25 -0600 Subject: signal/ptrace: Add force_sig_ptrace_errno_trap and use it where needed There are so many places that build struct siginfo by hand that at least one of them is bound to get it wrong. A handful of cases in the kernel arguably did just that when using the errno field of siginfo to pass no errno values to userspace. The usage is limited to a single si_code so at least does not mess up anything else. Encapsulate this questionable pattern in a helper function so that the userspace ABI is preserved. Update all of the places that use this pattern to use the new helper function. Signed-off-by: "Eric W. Biederman" --- arch/arm64/kernel/ptrace.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'arch/arm64/kernel') diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 95daa1478a7c..6618036ae6d4 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -190,21 +190,23 @@ static void ptrace_hbptriggered(struct perf_event *bp, #ifdef CONFIG_COMPAT if (is_compat_task()) { + int si_errno = 0; int i; for (i = 0; i < ARM_MAX_BRP; ++i) { if (current->thread.debug.hbp_break[i] == bp) { - info.si_errno = (i << 1) + 1; + si_errno = (i << 1) + 1; break; } } for (i = 0; i < ARM_MAX_WRP; ++i) { if (current->thread.debug.hbp_watch[i] == bp) { - info.si_errno = -((i << 1) + 1); + si_errno = -((i << 1) + 1); break; } } + force_sig_ptrace_errno_trap(si_errno, (void __user *)bkpt->trigger); } #endif force_sig_info(SIGTRAP, &info, current); -- cgit v1.2.3