[PATCH 0/4] arch/arm: support seccomp

[PATCH 0/4] arch/arm: support seccomp

[Kees Cook]

This adds support for seccomp BPF to ARM. When built with the seccomp
improvement patch waiting in linux-next ("seccomp: Make syscall skipping
and nr changes more consistent"), this passes the seccomp regression
test suite: https://github.com/redpig/seccomp

Thanks,

-Kees

[PATCH 1/4] arch/arm: add syscall_get_arch

[Kees Cook]

From: Will Drewry <wad@chromium.org>

Provide an ARM implementation of syscall_get_arch. This is a pre-requisite
for CONFIG_HAVE_ARCH_SECCOMP_FILTER.

Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/include/asm/syscall.h |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/include/asm/syscall.h b/arch/arm/include/asm/syscall.h
index 9fdded6..803f433 100644
--- a/arch/arm/include/asm/syscall.h
+++ b/arch/arm/include/asm/syscall.h
@@ -7,6 +7,8 @@
 #ifndef _ASM_ARM_SYSCALL_H
 #define _ASM_ARM_SYSCALL_H
 
+#include <linux/audit.h> /* for AUDIT_ARCH_* */
+#include <linux/elf.h> /* for ELF_EM */
 #include <linux/err.h>
 #include <linux/sched.h>
 
@@ -95,4 +97,15 @@ static inline void syscall_set_arguments(struct task_struct *task,
 	memcpy(&regs->ARM_r0 + i, args, n * sizeof(args[0]));
 }
 
+static inline int syscall_get_arch(struct task_struct *task,
+				   struct pt_regs *regs)
+{
+	/* ARM tasks don't change audit architectures on the fly. */
+#ifdef __ARMEB__
+	return AUDIT_ARCH_ARMEB;
+#else
+	return AUDIT_ARCH_ARM;
+#endif
+}
+
 #endif /* _ASM_ARM_SYSCALL_H */
-- 
1.7.9.5

[PATCH 2/4] arch/arm: move secure_computing into trace

[Kees Cook]

From: Will Drewry <wad@chromium.org>

There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_TRACE
path in entry-common.S. In order to add support for
CONFIG_HAVE_ARCH_SECCOMP_FILTER without mangling the assembly too badly,
seccomp was moved into the syscall_trace_enter() handler.

Additionally, the return value for secure_computing() is now checked
and a -1 value will result in the system call being skipped.

Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/entry-common.S |    9 ++-------
 arch/arm/kernel/ptrace.c       |    3 +++
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 3471175..c781012 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -418,13 +418,8 @@ local_restart:
 	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:
+	tst	r10, #_TIF_SECCOMP		@ is seccomp enabled?
+	bne	__sys_trace
 #endif
 
 	tst	r10, #_TIF_SYSCALL_WORK		@ are we tracing syscalls?
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 739db3a..aa4d93f 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -923,6 +923,9 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
 
 	current_thread_info()->syscall = scno;
 
+	if (dir == PTRACE_SYSCALL_ENTER && secure_computing(scno) == -1)
+		return -1;
+
 	if (!test_thread_flag(TIF_SYSCALL_TRACE))
 		return scno;
 
-- 
1.7.9.5

[PATCH 3/4] arch/arm: allow a scno of -1 to not cause a SIGILL

[Kees Cook]

From: Will Drewry <wad@chromium.org>

On tracehook-friendly platforms, a system call number of -1 falls
through without running much code or taking much action.

ARM is different.  This adds a lightweight check to arm_syscall()
to make sure that ARM behaves the same way.

Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/traps.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index b0179b8..f303ea6 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -540,6 +540,10 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
 	struct thread_info *thread = current_thread_info();
 	siginfo_t info;
 
+	/* Emulate/fallthrough. */
+	if (no == -1)
+		return regs->ARM_r0;
+
 	if ((no >> 16) != (__ARM_NR_BASE>> 16))
 		return bad_syscall(no, regs);
 
-- 
1.7.9.5

[PATCH 4/4] arch/arm: select HAVE_ARCH_SECCOMP_FILTER

[Kees Cook]

From: Will Drewry <wad@chromium.org>

Reflect architectural support for seccomp filter.

Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index ade7e92..0e8d490 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -21,6 +21,7 @@ config ARM
 	select HAVE_AOUT
 	select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL
 	select HAVE_ARCH_KGDB
+	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_BPF_JIT
 	select HAVE_C_RECORDMCOUNT
-- 
1.7.9.5

Re: [PATCH 2/4] arch/arm: move secure_computing into trace

[Al Viro]

On Mon, Oct 29, 2012 at 05:41:20PM -0700, Kees Cook wrote:
> From: Will Drewry <wad@chromium.org>
> 
> There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_TRACE
> path in entry-common.S. In order to add support for
> CONFIG_HAVE_ARCH_SECCOMP_FILTER without mangling the assembly too badly,
> seccomp was moved into the syscall_trace_enter() handler.
> 
> Additionally, the return value for secure_computing() is now checked
> and a -1 value will result in the system call being skipped.

This is too ugly.  Just expand the calls of ptrace_syscall_trace() into
both callers and do secure_computing() hookup in there.  And for pity
sake, would somebody rename the damn thing?  It's *dripping* with
marketdroidese...

Re: [PATCH 2/4] arch/arm: move secure_computing into trace

[Kees Cook]

On Mon, Oct 29, 2012 at 7:05 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Oct 29, 2012 at 05:41:20PM -0700, Kees Cook wrote:
>> From: Will Drewry <wad@chromium.org>
>>
>> There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_TRACE
>> path in entry-common.S. In order to add support for
>> CONFIG_HAVE_ARCH_SECCOMP_FILTER without mangling the assembly too badly,
>> seccomp was moved into the syscall_trace_enter() handler.
>>
>> Additionally, the return value for secure_computing() is now checked
>> and a -1 value will result in the system call being skipped.
>
> This is too ugly.  Just expand the calls of ptrace_syscall_trace() into
> both callers and do secure_computing() hookup in there.  And for pity

So ad722541 didn't go far enough? It seems like it makes sense to
re-use the code in there.

> sake, would somebody rename the damn thing?  It's *dripping* with
> marketdroidese...

True, but that's been its name since seccomp mode 1. We could rename
it internally, but I think that would make more sense as a separate
patch set.

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH 3/4] arch/arm: allow a scno of -1 to not cause a SIGILL

[Kees Cook]

On tracehook-friendly platforms, a system call number of -1 falls
through without running much code or taking much action.

ARM is different. This adds a short-circuit check in the trace path to
avoid any additional work, as suggested by Russell King, to make sure
that ARM behaves the same way as other platforms.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/entry-common.S |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 8355d4b..0bef977 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -448,7 +448,10 @@ __sys_trace:
 	ldmccia	r1, {r0 - r6}			@ have to reload r0 - r6
 	stmccia	sp, {r4, r5}			@ and update the stack args
 	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
-- 
1.7.9.5

[PATCH 3/4] arch/arm: allow a scno of -1 to not cause a SIGILL

[Kees Cook]

On tracehook-friendly platforms, a system call number of -1 falls
through without running much code or taking much action.

ARM is different. This adds a short-circuit check in the trace path to
avoid any additional work, as suggested by Russell King, to make sure
that ARM behaves the same way as other platforms.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/entry-common.S |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 8355d4b..0bef977 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -448,7 +448,10 @@ __sys_trace:
 	ldmccia	r1, {r0 - r6}			@ have to reload r0 - r6
 	stmccia	sp, {r4, r5}			@ and update the stack args
 	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
-- 
1.7.9.5

[PATCH 3/4] arch/arm: allow a scno of -1 to not cause a SIGILL

[Kees Cook]

On tracehook-friendly platforms, a system call number of -1 falls
through without running much code or taking much action.

ARM is different. This adds a short-circuit check in the trace path to
avoid any additional work, as suggested by Russell King, to make sure
that ARM behaves the same way as other platforms.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/entry-common.S |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 8355d4b..0bef977 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -448,7 +448,10 @@ __sys_trace:
 	ldmccia	r1, {r0 - r6}			@ have to reload r0 - r6
 	stmccia	sp, {r4, r5}			@ and update the stack args
 	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
-- 
1.7.9.5

Re: [PATCH 3/4] arch/arm: allow a scno of -1 to not cause a SIGILL

[Kees Cook]

On Thu, Nov 1, 2012 at 1:25 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Nov 01, 2012 at 12:46:38PM -0700, Kees Cook wrote:
>> From: Will Drewry <wad@chromium.org>
>>
>> On tracehook-friendly platforms, a system call number of -1 falls
>> through without running much code or taking much action.
>>
>> ARM is different.  This adds a lightweight check to arm_syscall()
>> to make sure that ARM behaves the same way.
>>
>> Signed-off-by: Will Drewry <wad@chromium.org>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  arch/arm/kernel/traps.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>> index b0179b8..f303ea6 100644
>> --- a/arch/arm/kernel/traps.c
>> +++ b/arch/arm/kernel/traps.c
>> @@ -540,6 +540,10 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
>>       struct thread_info *thread = current_thread_info();
>>       siginfo_t info;
>>
>> +     /* Emulate/fallthrough. */
>> +     if (no == -1)
>> +             return regs->ARM_r0;
>> +
>
> This won't work properly with OABI.  The problem is that OABI has an
> offset on its syscall numbers which is removed/added at appropriate
> times, and this is one of the places where it's put back.  So you end
> up with -1 XOR 0x900000 here, not -1.
>
> It'd probably be better to do this check in the asm code here, which
> prevents that yuckyness from affecting this.
>
> __sys_trace:
>         mov     r1, scno
>         add     r0, sp, #S_OFF
>         bl      syscall_trace_enter
>
>         adr     lr, BSYM(__sys_trace_return)    @ return address
>         mov     scno, r0                        @ syscall number (possibly new)
>         add     r1, sp, #S_R0 + S_OFF           @ pointer to regs
>         cmp     scno, #NR_syscalls              @ check upper syscall limit
>         ldmccia r1, {r0 - r6}                   @ have to reload r0 - r6
>         stmccia sp, {r4, r5}                    @ and update the stack args
>         ldrcc   pc, [tbl, scno, lsl #2]         @ call sys_* routine
> +       cmp     scno, #-1
>         bne     2b
> +       b       ret_slow_syscall
>

Ah! Good call, yes. I'll use this and include it in a v3 posting. Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 3/4] arch/arm: allow a scno of -1 to not cause a SIGILL

[Russell King - ARM Linux]

On Thu, Nov 01, 2012 at 12:46:38PM -0700, Kees Cook wrote:
> From: Will Drewry <wad@chromium.org>
> 
> On tracehook-friendly platforms, a system call number of -1 falls
> through without running much code or taking much action.
> 
> ARM is different.  This adds a lightweight check to arm_syscall()
> to make sure that ARM behaves the same way.
> 
> Signed-off-by: Will Drewry <wad@chromium.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/arm/kernel/traps.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index b0179b8..f303ea6 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -540,6 +540,10 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
>  	struct thread_info *thread = current_thread_info();
>  	siginfo_t info;
>  
> +	/* Emulate/fallthrough. */
> +	if (no == -1)
> +		return regs->ARM_r0;
> +

This won't work properly with OABI.  The problem is that OABI has an
offset on its syscall numbers which is removed/added at appropriate
times, and this is one of the places where it's put back.  So you end
up with -1 XOR 0x900000 here, not -1.

It'd probably be better to do this check in the asm code here, which
prevents that yuckyness from affecting this.

__sys_trace:
        mov     r1, scno
        add     r0, sp, #S_OFF
        bl      syscall_trace_enter

        adr     lr, BSYM(__sys_trace_return)    @ return address
        mov     scno, r0                        @ syscall number (possibly new)
        add     r1, sp, #S_R0 + S_OFF           @ pointer to regs
        cmp     scno, #NR_syscalls              @ check upper syscall limit
        ldmccia r1, {r0 - r6}                   @ have to reload r0 - r6
        stmccia sp, {r4, r5}                    @ and update the stack args
        ldrcc   pc, [tbl, scno, lsl #2]         @ call sys_* routine
+	cmp	scno, #-1
        bne     2b
+	b	ret_slow_syscall

[PATCH 3/4] arch/arm: allow a scno of -1 to not cause a SIGILL

[Kees Cook]

From: Will Drewry <wad@chromium.org>

On tracehook-friendly platforms, a system call number of -1 falls
through without running much code or taking much action.

ARM is different.  This adds a lightweight check to arm_syscall()
to make sure that ARM behaves the same way.

Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/traps.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index b0179b8..f303ea6 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -540,6 +540,10 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
 	struct thread_info *thread = current_thread_info();
 	siginfo_t info;
 
+	/* Emulate/fallthrough. */
+	if (no == -1)
+		return regs->ARM_r0;
+
 	if ((no >> 16) != (__ARM_NR_BASE>> 16))
 		return bad_syscall(no, regs);
 
-- 
1.7.9.5