| Message ID | 20131106205710.GN4994@outflux.net |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 06, 2013 at 12:57:10PM -0800, Kees Cook wrote: > This is a behavioral backport of the upstream ARM seccomp-bpf support, > with as few changes as possible. This passes the seccomp test suite on > both x86 and ARM. Could you drop in a pointer to this test suite for us, we might well be interested in using that. Could you confirm that this should not affect operations but is mostly fixing up holes that might be exploitable. Otherwise I assume what this does is bring what is in Precise up to the same level as mainline, allowing us to guarentee that more modern consumers of this interface will work consistantly on all releases back to Precice. Thanks! -apw
Hi, On Thu, Nov 7, 2013 at 6:32 AM, Andy Whitcroft <apw@canonical.com> wrote: > On Wed, Nov 06, 2013 at 12:57:10PM -0800, Kees Cook wrote: >> This is a behavioral backport of the upstream ARM seccomp-bpf support, >> with as few changes as possible. This passes the seccomp test suite on >> both x86 and ARM. > > Could you drop in a pointer to this test suite for us, we might well be > interested in using that. Sure! I actually already added that to the bug[1] report, since that seemed the right place to collect the SRU details. Repeating it here for good measure: [Test Case] git clone https://github.com/redpig/seccomp.git cd seccomp/tests make ./seccomp_bpf_tests All tests should pass > Could you confirm that this should not affect operations but is mostly > fixing up holes that might be exploitable. It does not change x86 operation, and does not fix exploitable holes. It simply makes seccomp-bpf available at all on ARM. Before this patch, seccomp-bpf couldn't be used on ARM. The driving motivation here is to get Chrome on ARM on Precise making use of its full sandboxing capabilities. > Otherwise I assume what this does is bring what is in Precise up to the > same level as mainline, allowing us to guarentee that more modern > consumers of this interface will work consistantly on all releases back > to Precice. Correct -- in the core seccomp, it fixes 1 extremely minor difference between Precise and mainline (the action masks), but that change has no behavioral difference for "real" seccomp filters, and the old situation posed no risk to the system for a "bad" seccomp filter. It was simply noticed during the much more strict test case execution. Besides that, it just wires up everything that is required on ARM to call into seccomp correctly. -Kees [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1183616
Bad news. This is the highbank flavour compiled on native armhf. kernel/seccomp.c:28:25: fatal error: asm/syscall.h: No such file or directory compilation terminated. make[1]: *** [kernel/seccomp.o] Error 1 make: *** [_module_kernel] Error 2
On Fri, Nov 8, 2013 at 1:19 PM, Tim Gardner <tim.gardner@canonical.com> wrote: > Bad news. This is the highbank flavour compiled on native armhf. Errrg. > kernel/seccomp.c:28:25: fatal error: asm/syscall.h: No such file or > directory > compilation terminated. > make[1]: *** [kernel/seccomp.o] Error 1 > make: *** [_module_kernel] Error 2 > -- > Tim Gardner tim.gardner@canonical.com Did all the syscall backporting work from the main Ubuntu kernel tree not end up in the highbank tree? Regardless, if you can point me to the tree, I can find the missing commits. -Kees
On 11/08/2013 01:23 PM, Kees Cook wrote: > On Fri, Nov 8, 2013 at 1:19 PM, Tim Gardner <tim.gardner@canonical.com> wrote: >> Bad news. This is the highbank flavour compiled on native armhf. > > Errrg. > >> kernel/seccomp.c:28:25: fatal error: asm/syscall.h: No such file or >> directory >> compilation terminated. >> make[1]: *** [kernel/seccomp.o] Error 1 >> make: *** [_module_kernel] Error 2 >> -- >> Tim Gardner tim.gardner@canonical.com > > Did all the syscall backporting work from the main Ubuntu kernel tree > not end up in the highbank tree? > > Regardless, if you can point me to the tree, I can find the missing commits. > > -Kees > git://kernel.ubuntu.com/ubuntu/ubuntu-precise.git
On Fri, Nov 8, 2013 at 1:33 PM, Tim Gardner <tim.gardner@canonical.com> wrote: > On 11/08/2013 01:23 PM, Kees Cook wrote: >> On Fri, Nov 8, 2013 at 1:19 PM, Tim Gardner <tim.gardner@canonical.com> wrote: >>> Bad news. This is the highbank flavour compiled on native armhf. >> >> Errrg. >> >>> kernel/seccomp.c:28:25: fatal error: asm/syscall.h: No such file or >>> directory >>> compilation terminated. >>> make[1]: *** [kernel/seccomp.o] Error 1 >>> make: *** [_module_kernel] Error 2 >>> -- >>> Tim Gardner tim.gardner@canonical.com >> >> Did all the syscall backporting work from the main Ubuntu kernel tree >> not end up in the highbank tree? >> >> Regardless, if you can point me to the tree, I can find the missing commits. >> >> -Kees >> > > git://kernel.ubuntu.com/ubuntu/ubuntu-precise.git That is very odd. I have 2 commits in my check out of that tree that I don't see in there. Perhaps I was building on my prior attempt and didn't realize it. Ugh, yeah, looks like I accidentally pulled in two patches months ago in my "master" check out. One moment, I'll clean this up and send a pull request with the 3 pieces. Sorry about that! -Kees
On Thu, Nov 07, 2013 at 09:21:40AM -0800, Kees Cook wrote: > Sure! I actually already added that to the bug[1] report, since that > seemed the right place to collect the SRU details. Repeating it here > for good measure: > > [Test Case] > git clone https://github.com/redpig/seccomp.git > cd seccomp/tests > make > ./seccomp_bpf_tests > All tests should pass > > > Could you confirm that this should not affect operations but is mostly > > fixing up holes that might be exploitable. > > It does not change x86 operation, and does not fix exploitable holes. > It simply makes seccomp-bpf available at all on ARM. Before this > patch, seccomp-bpf couldn't be used on ARM. The driving motivation > here is to get Chrome on ARM on Precise making use of its full > sandboxing capabilities. > > > Otherwise I assume what this does is bring what is in Precise up to the > > same level as mainline, allowing us to guarentee that more modern > > consumers of this interface will work consistantly on all releases back > > to Precice. > > Correct -- in the core seccomp, it fixes 1 extremely minor difference > between Precise and mainline (the action masks), but that change has > no behavioral difference for "real" seccomp filters, and the old > situation posed no risk to the system for a "bad" seccomp filter. It > was simply noticed during the much more strict test case execution. > > Besides that, it just wires up everything that is required on ARM to > call into seccomp correctly. > > -Kees > > [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1183616 Thanks for all those juicy details. Just what I was trying to understand. -apw
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index a78240d..0ae6743 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -12,6 +12,7 @@ config ARM select HAVE_KPROBES if !XIP_KERNEL select HAVE_KRETPROBES if (HAVE_KPROBES) select HAVE_FUNCTION_TRACER if (!XIP_KERNEL) + select HAVE_ARCH_SECCOMP_FILTER select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL) select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) select HAVE_FUNCTION_GRAPH_TRACER if (!THUMB2_KERNEL) diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h index 7b5cc8d..a483545 100644 --- a/arch/arm/include/asm/thread_info.h +++ b/arch/arm/include/asm/thread_info.h @@ -139,12 +139,12 @@ extern void vfp_flush_hwstate(struct thread_info *); #define TIF_NEED_RESCHED 1 #define TIF_NOTIFY_RESUME 2 /* callback before returning to user */ #define TIF_SYSCALL_TRACE 8 +#define TIF_SECCOMP 11 /* seccomp syscall filtering active */ #define TIF_POLLING_NRFLAG 16 #define TIF_USING_IWMMXT 17 #define TIF_MEMDIE 18 /* is terminating due to OOM killer */ #define TIF_FREEZE 19 #define TIF_RESTORE_SIGMASK 20 -#define TIF_SECCOMP 21 #define _TIF_SIGPENDING (1 << TIF_SIGPENDING) #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) @@ -156,6 +156,9 @@ extern void vfp_flush_hwstate(struct thread_info *); #define _TIF_RESTORE_SIGMASK (1 << TIF_RESTORE_SIGMASK) #define _TIF_SECCOMP (1 << TIF_SECCOMP) +/* Checks for any syscall work in entry-common.S */ +#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SECCOMP) + /* * Change these and you break ASM code in entry-common.S */ diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S index b2a27b6..82f8fe1 100644 --- a/arch/arm/kernel/entry-common.S +++ b/arch/arm/kernel/entry-common.S @@ -433,17 +433,7 @@ ENTRY(vector_swi) ldr r10, [tsk, #TI_FLAGS] @ check for syscall tracing stmdb sp!, {r4, r5} @ push fifth and sixth args -#ifdef CONFIG_SECCOMP - tst r10, #_TIF_SECCOMP - beq 1f - mov r0, scno - bl __secure_computing - add r0, sp, #S_R0 + S_OFF @ pointer to regs - ldmia r0, {r0 - r3} @ have to reload r0 - r3 -1: -#endif - - tst r10, #_TIF_SYSCALL_TRACE @ are we tracing syscalls? + tst r10, #_TIF_SYSCALL_WORK @ are we tracing syscalls? bne __sys_trace cmp scno, #NR_syscalls @ check upper syscall limit @@ -474,7 +464,10 @@ __sys_trace: cmp scno, #NR_syscalls @ check upper syscall limit ldmccia r1, {r0 - r3} @ have to reload r0 - r3 ldrcc pc, [tbl, scno, lsl #2] @ call sys_* routine - b 2b + cmp scno, #-1 @ skip the syscall? + bne 2b + add sp, sp, #S_OFF @ restore stack + b ret_slow_syscall __sys_trace_return: str r0, [sp, #S_R0 + S_OFF]! @ save returned r0 diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c index 90fa8b3..0ebff48 100644 --- a/arch/arm/kernel/ptrace.c +++ b/arch/arm/kernel/ptrace.c @@ -908,6 +908,15 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno) { unsigned long ip; + current_thread_info()->syscall = scno; + + /* + * Only check seccomp on entry. Do the secure computing check first; + * failures should be fast. + */ + if (why == 0 && secure_computing(scno) == -1) + return -1; + if (!test_thread_flag(TIF_SYSCALL_TRACE)) return scno; if (!(current->ptrace & PT_PTRACED)) @@ -920,8 +929,6 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int scno) ip = regs->ARM_ip; regs->ARM_ip = why; - current_thread_info()->syscall = scno; - /* the 0x80 provides a way for the tracing parent to distinguish between a syscall stop and SIGTRAP delivery */ ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD) diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h index 306733e..41ff13b 100644 --- a/include/linux/seccomp.h +++ b/include/linux/seccomp.h @@ -25,7 +25,7 @@ #define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */ /* Masks for the return value sections. */ -#define SECCOMP_RET_ACTION 0xffff0000U +#define SECCOMP_RET_ACTION 0x7fff0000U #define SECCOMP_RET_DATA 0x0000ffffU /** diff --git a/kernel/seccomp.c b/kernel/seccomp.c index bc84054..533328d 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -414,8 +414,12 @@ int __secure_computing_int(int this_syscall) goto skip; case SECCOMP_RET_TRACE: /* Skip these calls if there is no tracer. */ - if (!ptrace_event_enabled(current, PTRACE_EVENT_SECCOMP)) + if (!ptrace_event_enabled(current, PTRACE_EVENT_SECCOMP)) { + /* Make sure userspace sees an ENOSYS. */ + syscall_set_return_value(current, + task_pt_regs(current), -ENOSYS, 0); goto skip; + } /* Allow the BPF to provide the event message */ ptrace_event(PTRACE_EVENT_SECCOMP, data); if (fatal_signal_pending(current))
This is a behavioral backport of the upstream ARM seccomp-bpf support, with as few changes as possible. This passes the seccomp test suite on both x86 and ARM. There is very little difference in the TIF_SYSCALL_TRACE and TIF_SECCOMP path in entry-common.S, so merge them into TIF_SYSCALL_WORK and move seccomp into the syscall_trace() handler. (TIF_SECCOMP renumbered to fit into an ARM instruction literal.) The return value for secure_computing() is now checked and a -1 value will result in the system call being skipped. An -ENOSYS is explicitly passed for these skipped syscalls, which is something not done automatically on ARM. The ARM thread's syscall information is needed earlier for seccomp, so its assignment has been moved up. The SECCOMP_RET_ACTION mask has been adjusted to match upstream; this change has no effect on real filters, but was needed to pass the strict test cases. BugLink: http://bugs.launchpad.net/bugs/1183616 Signed-off-by: Kees Cook <keescook@chromium.org> --- arch/arm/Kconfig | 1 + arch/arm/include/asm/thread_info.h | 5 ++++- arch/arm/kernel/entry-common.S | 17 +++++------------ arch/arm/kernel/ptrace.c | 11 +++++++++-- include/linux/seccomp.h | 2 +- kernel/seccomp.c | 6 +++++- 6 files changed, 25 insertions(+), 17 deletions(-)