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.
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 <[email protected]>
---
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
From: Will Drewry <[email protected]>
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 <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
From: Will Drewry <[email protected]>
Provide an ARM implementation of syscall_get_arch. This is a pre-requisite
for CONFIG_HAVE_ARCH_SECCOMP_FILTER.
Signed-off-by: Will Drewry <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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(®s->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
From: Will Drewry <[email protected]>
Reflect architectural support for seccomp filter.
Signed-off-by: Will Drewry <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
On Thu, Nov 01, 2012 at 12:46:38PM -0700, Kees Cook wrote:
> From: Will Drewry <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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
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
>
On Thu, Nov 1, 2012 at 1:33 PM, Russell King - ARM Linux
<[email protected]> 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
On Thu, Nov 1, 2012 at 1:25 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Thu, Nov 01, 2012 at 12:46:38PM -0700, Kees Cook wrote:
>> From: Will Drewry <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Kees Cook <[email protected]>
>> ---
>> 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