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

[PATCH v2 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

---
v2:
 - expanded ptrace_syscall_trace() into both callers and do
   secure_computing() hookup there, as requested by Al Viro.

[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]

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.

Expanded ptrace_syscall_trace() into both callers to make code more
readable, as requested by Al Viro.

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: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/entry-common.S |    9 ++-------
 arch/arm/kernel/ptrace.c       |   39 +++++++++++++++++++++++++--------------
 2 files changed, 27 insertions(+), 21 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..6b0e14b 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -916,13 +916,15 @@ enum ptrace_syscall_dir {
 	PTRACE_SYSCALL_EXIT,
 };
 
-static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
-				enum ptrace_syscall_dir dir)
+asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 {
 	unsigned long ip;
 
 	current_thread_info()->syscall = scno;
 
+	if (secure_computing(scno) == -1)
+		return -1;
+
 	if (!test_thread_flag(TIF_SYSCALL_TRACE))
 		return scno;
 
@@ -931,20 +933,13 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
 	 * IP = 0 -> entry, =1 -> exit
 	 */
 	ip = regs->ARM_ip;
-	regs->ARM_ip = dir;
-
-	if (dir == PTRACE_SYSCALL_EXIT)
-		tracehook_report_syscall_exit(regs, 0);
-	else if (tracehook_report_syscall_entry(regs))
+	regs->ARM_ip = PTRACE_SYSCALL_ENTER;
+	if (tracehook_report_syscall_entry(regs))
 		current_thread_info()->syscall = -1;
-
 	regs->ARM_ip = ip;
-	return current_thread_info()->syscall;
-}
 
-asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
-{
-	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
+	scno = current_thread_info()->syscall;
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_enter(regs, scno);
 	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
@@ -954,7 +949,23 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 
 asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
 {
-	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_EXIT);
+	unsigned long ip;
+
+	current_thread_info()->syscall = scno;
+
+	if (!test_thread_flag(TIF_SYSCALL_TRACE))
+		return scno;
+
+	/*
+	 * IP is used to denote syscall entry/exit:
+	 * IP = 0 -> entry, =1 -> exit
+	 */
+	ip = regs->ARM_ip;
+	regs->ARM_ip = PTRACE_SYSCALL_EXIT;
+	tracehook_report_syscall_exit(regs, 0);
+	regs->ARM_ip = ip;
+
+	scno = current_thread_info()->syscall;
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_exit(regs, scno);
 	audit_syscall_exit(regs);
-- 
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 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

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

[Russell King - ARM Linux]

On Thu, Nov 01, 2012 at 12:46:37PM -0700, Kees Cook wrote:
>  #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?

It's pointless having:

	tst	r10, #_TIF_SECCOMP
	bne	__sys_trace
	tst	r10, #_TIF_SYSCALL_WORK
	bne	__sys_trace

Instead, make TIF_SECCOMP be bit 11, combine it into _TIF_SYSCALL_WORK, and
eliminate all of that CONFIG_SECCOMP block.

> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index 739db3a..6b0e14b 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -916,13 +916,15 @@ enum ptrace_syscall_dir {
>  	PTRACE_SYSCALL_EXIT,
>  };
>  
> -static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
> -				enum ptrace_syscall_dir dir)
> +asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
>  {
>  	unsigned long ip;
>  
>  	current_thread_info()->syscall = scno;
>  
> +	if (secure_computing(scno) == -1)
> +		return -1;
> +
>  	if (!test_thread_flag(TIF_SYSCALL_TRACE))
>  		return scno;

I'm not sure this change is correct (combined with your hunk below).
What if we have auditing enabled but trace disabled?  How do we reach
audit_syscall_entry()?  Or the tracehook stuff?

This patch looks wrong in too many ways.

> @@ -931,20 +933,13 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
>  	 * IP = 0 -> entry, =1 -> exit
>  	 */
>  	ip = regs->ARM_ip;
> -	regs->ARM_ip = dir;
> -
> -	if (dir == PTRACE_SYSCALL_EXIT)
> -		tracehook_report_syscall_exit(regs, 0);
> -	else if (tracehook_report_syscall_entry(regs))
> +	regs->ARM_ip = PTRACE_SYSCALL_ENTER;
> +	if (tracehook_report_syscall_entry(regs))
>  		current_thread_info()->syscall = -1;
> -
>  	regs->ARM_ip = ip;
> -	return current_thread_info()->syscall;
> -}
>  
> -asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
> -{
> -	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
> +	scno = current_thread_info()->syscall;
> +
>  	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
>  		trace_sys_enter(regs, scno);
>  	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
> @@ -954,7 +949,23 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
>  
>  asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
>  {
> -	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_EXIT);
> +	unsigned long ip;
> +
> +	current_thread_info()->syscall = scno;
> +
> +	if (!test_thread_flag(TIF_SYSCALL_TRACE))
> +		return scno;
> +
> +	/*
> +	 * IP is used to denote syscall entry/exit:
> +	 * IP = 0 -> entry, =1 -> exit
> +	 */
> +	ip = regs->ARM_ip;
> +	regs->ARM_ip = PTRACE_SYSCALL_EXIT;
> +	tracehook_report_syscall_exit(regs, 0);
> +	regs->ARM_ip = ip;
> +
> +	scno = current_thread_info()->syscall;
>  	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
>  		trace_sys_exit(regs, scno);
>  	audit_syscall_exit(regs);
> -- 
> 1.7.9.5
> 

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

[Kees Cook]

On Thu, Nov 1, 2012 at 1:33 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> It's pointless having:
>
>         tst     r10, #_TIF_SECCOMP
>         bne     __sys_trace
>         tst     r10, #_TIF_SYSCALL_WORK
>         bne     __sys_trace
>
> Instead, make TIF_SECCOMP be bit 11, combine it into _TIF_SYSCALL_WORK, and
> eliminate all of that CONFIG_SECCOMP block.

Ah! Good point; I'd missed that _WORK was a bit field. I'll make those changes.

>> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
>> index 739db3a..6b0e14b 100644
>> --- a/arch/arm/kernel/ptrace.c
>> +++ b/arch/arm/kernel/ptrace.c
>> @@ -916,13 +916,15 @@ enum ptrace_syscall_dir {
>>       PTRACE_SYSCALL_EXIT,
>>  };
>>
>> -static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
>> -                             enum ptrace_syscall_dir dir)
>> +asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
>
> I'm not sure this change is correct (combined with your hunk below).
> What if we have auditing enabled but trace disabled?  How do we reach
> audit_syscall_entry()?  Or the tracehook stuff?
>
> This patch looks wrong in too many ways.

Oh, yeah, you're totally right. I will fix that up. Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

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

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

[Kees Cook]

There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_WORK
path in entry-common.S, so merge TIF_SECCOMP into TIF_SYSCALL_WORK and
move seccomp into the syscall_trace_enter() handler.

Expanded some of the tracehook logic into the callers to make this code
more readable. Since tracehook needs to do register changing, this portion
is best left in its own function instead of copy/pasting into the callers.

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: Kees Cook <keescook@chromium.org>
---
 arch/arm/include/asm/thread_info.h |    7 ++++---
 arch/arm/kernel/entry-common.S     |   10 ----------
 arch/arm/kernel/ptrace.c           |   29 ++++++++++++++++++++---------
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 8477b4c..cddda1f 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -151,10 +151,10 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define TIF_SYSCALL_TRACE	8
 #define TIF_SYSCALL_AUDIT	9
 #define TIF_SYSCALL_TRACEPOINT	10
+#define TIF_SECCOMP		11	/* seccomp syscall filtering active */
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_RESTORE_SIGMASK	20
-#define TIF_SECCOMP		21
 #define TIF_SWITCH_MM		22	/* deferred switch_mm */
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -163,11 +163,12 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
-#define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 
 /* Checks for any syscall work in entry-common.S */
-#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | _TIF_SYSCALL_TRACEPOINT)
+#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
+			   _TIF_SYSCALL_TRACEPOINT | _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 3471175..8355d4b 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -417,16 +417,6 @@ local_restart:
 	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_WORK		@ are we tracing syscalls?
 	bne	__sys_trace
 
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 739db3a..518536d 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -916,16 +916,11 @@ enum ptrace_syscall_dir {
 	PTRACE_SYSCALL_EXIT,
 };
 
-static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
-				enum ptrace_syscall_dir dir)
+static int tracehook_report_syscall(struct pt_regs *regs,
+				    enum ptrace_syscall_dir dir)
 {
 	unsigned long ip;
 
-	current_thread_info()->syscall = scno;
-
-	if (!test_thread_flag(TIF_SYSCALL_TRACE))
-		return scno;
-
 	/*
 	 * IP is used to denote syscall entry/exit:
 	 * IP = 0 -> entry, =1 -> exit
@@ -944,19 +939,35 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
 
 asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 {
-	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
+	current_thread_info()->syscall = scno;
+
+	/* Do the secure computing check first; failures should be fast. */
+	if (secure_computing(scno) == -1)
+		return -1;
+
+	if (test_thread_flag(TIF_SYSCALL_TRACE))
+		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_enter(regs, scno);
+
 	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
 			    regs->ARM_r2, regs->ARM_r3);
+
 	return scno;
 }
 
 asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
 {
-	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_EXIT);
+	current_thread_info()->syscall = scno;
+
+	if (test_thread_flag(TIF_SYSCALL_TRACE))
+		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_exit(regs, scno);
+
 	audit_syscall_exit(regs);
+
 	return scno;
 }
-- 
1.7.9.5

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

[Will Deacon]

One really minor nit...

On Thu, Nov 08, 2012 at 08:59:31PM +0000, Kees Cook wrote:
> There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_WORK
> path in entry-common.S, so merge TIF_SECCOMP into TIF_SYSCALL_WORK and
> move seccomp into the syscall_trace_enter() handler.
> 
> Expanded some of the tracehook logic into the callers to make this code
> more readable. Since tracehook needs to do register changing, this portion
> is best left in its own function instead of copy/pasting into the callers.
> 
> 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: Kees Cook <keescook@chromium.org>

[...]

> @@ -944,19 +939,39 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
>  
>  asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
>  {
> -	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
> +	current_thread_info()->syscall = scno;
> +
> +	/* do the secure computing check first */
> +	if (secure_computing(scno) == -1) {
> +		/* seccomp failures shouldn't expose any additional code. */
> +		scno = -1;
> +		goto out;
> +	}

Can we just return -1 here instead please? The whole jump label code makes
this code messier than it needs to be and there's no cleanup to be done.

> +	if (test_thread_flag(TIF_SYSCALL_TRACE))
> +		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
> +
>  	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
>  		trace_sys_enter(regs, scno);
> +
>  	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
>  			    regs->ARM_r2, regs->ARM_r3);
> +
> +out:
>  	return scno;
>  }

Cheers,

Will

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

[Kees Cook]

There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_WORK
path in entry-common.S, so merge TIF_SECCOMP into TIF_SYSCALL_WORK and
move seccomp into the syscall_trace_enter() handler.

Expanded some of the tracehook logic into the callers to make this code
more readable. Since tracehook needs to do register changing, this portion
is best left in its own function instead of copy/pasting into the callers.

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: Kees Cook <keescook@chromium.org>
---
 arch/arm/include/asm/thread_info.h |    7 ++++---
 arch/arm/kernel/entry-common.S     |   10 ----------
 arch/arm/kernel/ptrace.c           |   33 ++++++++++++++++++++++++---------
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 8477b4c..cddda1f 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -151,10 +151,10 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define TIF_SYSCALL_TRACE	8
 #define TIF_SYSCALL_AUDIT	9
 #define TIF_SYSCALL_TRACEPOINT	10
+#define TIF_SECCOMP		11	/* seccomp syscall filtering active */
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_RESTORE_SIGMASK	20
-#define TIF_SECCOMP		21
 #define TIF_SWITCH_MM		22	/* deferred switch_mm */
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -163,11 +163,12 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
-#define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 
 /* Checks for any syscall work in entry-common.S */
-#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | _TIF_SYSCALL_TRACEPOINT)
+#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
+			   _TIF_SYSCALL_TRACEPOINT | _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 3471175..8355d4b 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -417,16 +417,6 @@ local_restart:
 	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_WORK		@ are we tracing syscalls?
 	bne	__sys_trace
 
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 739db3a..2aecf8f 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -916,16 +916,11 @@ enum ptrace_syscall_dir {
 	PTRACE_SYSCALL_EXIT,
 };
 
-static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
-				enum ptrace_syscall_dir dir)
+static int tracehook_report_syscall(struct pt_regs *regs,
+				    enum ptrace_syscall_dir dir)
 {
 	unsigned long ip;
 
-	current_thread_info()->syscall = scno;
-
-	if (!test_thread_flag(TIF_SYSCALL_TRACE))
-		return scno;
-
 	/*
 	 * IP is used to denote syscall entry/exit:
 	 * IP = 0 -> entry, =1 -> exit
@@ -944,19 +939,39 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
 
 asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 {
-	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
+	current_thread_info()->syscall = scno;
+
+	/* do the secure computing check first */
+	if (secure_computing(scno) == -1) {
+		/* seccomp failures shouldn't expose any additional code. */
+		scno = -1;
+		goto out;
+	}
+
+	if (test_thread_flag(TIF_SYSCALL_TRACE))
+		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_enter(regs, scno);
+
 	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
 			    regs->ARM_r2, regs->ARM_r3);
+
+out:
 	return scno;
 }
 
 asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
 {
-	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_EXIT);
+	current_thread_info()->syscall = scno;
+
+	if (test_thread_flag(TIF_SYSCALL_TRACE))
+		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_exit(regs, scno);
+
 	audit_syscall_exit(regs);
+
 	return scno;
 }
-- 
1.7.9.5

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

[Kees Cook]

There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_WORK
path in entry-common.S, so merge TIF_SECCOMP into TIF_SYSCALL_WORK and
move seccomp into the syscall_trace_enter() handler.

Expanded some of the tracehook logic into the callers to make this code
more readable. Since tracehook needs to do register changing, this portion
is best left in its own function instead of copy/pasting into the callers.

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: Kees Cook <keescook@chromium.org>
---
 arch/arm/include/asm/thread_info.h |    7 ++++---
 arch/arm/kernel/entry-common.S     |   10 ----------
 arch/arm/kernel/ptrace.c           |   33 ++++++++++++++++++++++++---------
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 8477b4c..cddda1f 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -151,10 +151,10 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define TIF_SYSCALL_TRACE	8
 #define TIF_SYSCALL_AUDIT	9
 #define TIF_SYSCALL_TRACEPOINT	10
+#define TIF_SECCOMP		11	/* seccomp syscall filtering active */
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
 #define TIF_RESTORE_SIGMASK	20
-#define TIF_SECCOMP		21
 #define TIF_SWITCH_MM		22	/* deferred switch_mm */
 
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -163,11 +163,12 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
-#define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
 
 /* Checks for any syscall work in entry-common.S */
-#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | _TIF_SYSCALL_TRACEPOINT)
+#define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
+			   _TIF_SYSCALL_TRACEPOINT | _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 3471175..8355d4b 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -417,16 +417,6 @@ local_restart:
 	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_WORK		@ are we tracing syscalls?
 	bne	__sys_trace
 
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 739db3a..2aecf8f 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -916,16 +916,11 @@ enum ptrace_syscall_dir {
 	PTRACE_SYSCALL_EXIT,
 };
 
-static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
-				enum ptrace_syscall_dir dir)
+static int tracehook_report_syscall(struct pt_regs *regs,
+				    enum ptrace_syscall_dir dir)
 {
 	unsigned long ip;
 
-	current_thread_info()->syscall = scno;
-
-	if (!test_thread_flag(TIF_SYSCALL_TRACE))
-		return scno;
-
 	/*
 	 * IP is used to denote syscall entry/exit:
 	 * IP = 0 -> entry, =1 -> exit
@@ -944,19 +939,39 @@ static int ptrace_syscall_trace(struct pt_regs *regs, int scno,
 
 asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 {
-	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_ENTER);
+	current_thread_info()->syscall = scno;
+
+	/* do the secure computing check first */
+	if (secure_computing(scno) == -1) {
+		/* seccomp failures shouldn't expose any additional code. */
+		scno = -1;
+		goto out;
+	}
+
+	if (test_thread_flag(TIF_SYSCALL_TRACE))
+		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_enter(regs, scno);
+
 	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
 			    regs->ARM_r2, regs->ARM_r3);
+
+out:
 	return scno;
 }
 
 asmlinkage int syscall_trace_exit(struct pt_regs *regs, int scno)
 {
-	scno = ptrace_syscall_trace(regs, scno, PTRACE_SYSCALL_EXIT);
+	current_thread_info()->syscall = scno;
+
+	if (test_thread_flag(TIF_SYSCALL_TRACE))
+		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_EXIT);
+
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
 		trace_sys_exit(regs, scno);
+
 	audit_syscall_exit(regs);
+
 	return scno;
 }
-- 
1.7.9.5

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

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...

[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