| Message ID | 20140618202748.GA9022@www.outflux.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 18, 2014 at 1:27 PM, Kees Cook <keescook@chromium.org> wrote: > An x86 tracer wanting to change the syscall uses PTRACE_SETREGS > (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL > (stored to current_thread_info()->syscall). When this happens, the > syscall can change across the call to secure_computing(), since it may > block on tracer notification, and the tracer can then make changes > to the process, before we return from secure_computing(). This > means the code must respect the changed syscall after the > secure_computing() call in syscall_trace_enter(). The same is true > for tracehook_report_syscall_entry() which may also block and change > the syscall. > > The x86 code handles this (it expects orig_ax to always be the > desired syscall). In the ARM case, this means we should not be touching > current_thread_info()->syscall after its initial assignment. All failures > should result in a -1 syscall, though. This looks sensible. --Andy
Hi Kees, I'm struggling to see the bug in the current code, so apologies if my questions aren't helpful. On Wed, Jun 18, 2014 at 09:27:48PM +0100, Kees Cook wrote: > An x86 tracer wanting to change the syscall uses PTRACE_SETREGS > (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL > (stored to current_thread_info()->syscall). When this happens, the > syscall can change across the call to secure_computing(), since it may > block on tracer notification, and the tracer can then make changes > to the process, before we return from secure_computing(). This > means the code must respect the changed syscall after the > secure_computing() call in syscall_trace_enter(). The same is true > for tracehook_report_syscall_entry() which may also block and change > the syscall. I don't think I understand what you mean by `the code must respect the changed syscall'. The current code does indeed issue the new syscall, so are you more concerned with secure_computing changing ->syscall, then the debugger can't see the new syscall when it sees the trap from tracehook? Are these even supposed to inter-operate? > The x86 code handles this (it expects orig_ax to always be the > desired syscall). In the ARM case, this means we should not be touching > current_thread_info()->syscall after its initial assignment. All failures > should result in a -1 syscall, though. The only time we explicitly touch ->syscall is when we're aborting the call (i.e. writing -1), which I think is fine. Will
On Fri, Jun 20, 2014 at 3:22 AM, Will Deacon <will.deacon@arm.com> wrote: > Hi Kees, > > I'm struggling to see the bug in the current code, so apologies if my > questions aren't helpful. > > On Wed, Jun 18, 2014 at 09:27:48PM +0100, Kees Cook wrote: >> An x86 tracer wanting to change the syscall uses PTRACE_SETREGS >> (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL >> (stored to current_thread_info()->syscall). When this happens, the >> syscall can change across the call to secure_computing(), since it may >> block on tracer notification, and the tracer can then make changes >> to the process, before we return from secure_computing(). This >> means the code must respect the changed syscall after the >> secure_computing() call in syscall_trace_enter(). The same is true >> for tracehook_report_syscall_entry() which may also block and change >> the syscall. > > I don't think I understand what you mean by `the code must respect the > changed syscall'. The current code does indeed issue the new syscall, so are > you more concerned with secure_computing changing ->syscall, then the > debugger can't see the new syscall when it sees the trap from tracehook? > Are these even supposed to inter-operate? The problem is the use of "scno" in the call -- it results in ignoring the value that may be set up in ->syscall by a tracer: syscall_trace_enter(regs, scno): current_thread_info()->syscall = scno; secure_computing(scno): [block on ptrace] [ptracer changes current_thread_info()->syscall vis PTRACE_SET_SYSCALL] ... return scno; This means the tracer's changed syscall value will be ignored (syscall_trace_enter returns original "scno" instead of actual current_thread_info()->syscall). In the original code, failure cases were propagated correctly, but not tracer-induced changes. Is that more clear? It's not an obvious state (due to the external modification of process state during the ptrace blocking). I've also got tests for this, if that's useful to further illustrate: https://github.com/kees/seccomp/commit/bd24e174593f79784b97178b583f17e0ea9d2aa7 > >> The x86 code handles this (it expects orig_ax to always be the >> desired syscall). In the ARM case, this means we should not be touching >> current_thread_info()->syscall after its initial assignment. All failures >> should result in a -1 syscall, though. > > The only time we explicitly touch ->syscall is when we're aborting the call > (i.e. writing -1), which I think is fine. That was the error path, which was okay. This rearranges things to both handle error conditions and tracer changes. -Kees
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c index 0dd3b79b15c3..97bd95f6aa01 100644 --- a/arch/arm/kernel/ptrace.c +++ b/arch/arm/kernel/ptrace.c @@ -911,6 +911,7 @@ enum ptrace_syscall_dir { static int tracehook_report_syscall(struct pt_regs *regs, enum ptrace_syscall_dir dir) { + int ret = 0; unsigned long ip; /* @@ -923,30 +924,35 @@ static int tracehook_report_syscall(struct pt_regs *regs, if (dir == PTRACE_SYSCALL_EXIT) tracehook_report_syscall_exit(regs, 0); else if (tracehook_report_syscall_entry(regs)) - current_thread_info()->syscall = -1; + ret = -1; regs->ARM_ip = ip; - return current_thread_info()->syscall; + return ret; } asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno) { + int ret = 0; + + /* set up syscall, which may be changed in secure_computing */ 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_TRACE) && + tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER)) + ret = -1; if (test_thread_flag(TIF_SYSCALL_TRACEPOINT)) - trace_sys_enter(regs, scno); + trace_sys_enter(regs, current_thread_info()->syscall); - audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1, + audit_syscall_entry(AUDIT_ARCH_ARM, current_thread_info()->syscall, + regs->ARM_r0, regs->ARM_r1, regs->ARM_r2, regs->ARM_r3); - return scno; + return ret ?: current_thread_info()->syscall; } asmlinkage void syscall_trace_exit(struct pt_regs *regs)
An x86 tracer wanting to change the syscall uses PTRACE_SETREGS (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL (stored to current_thread_info()->syscall). When this happens, the syscall can change across the call to secure_computing(), since it may block on tracer notification, and the tracer can then make changes to the process, before we return from secure_computing(). This means the code must respect the changed syscall after the secure_computing() call in syscall_trace_enter(). The same is true for tracehook_report_syscall_entry() which may also block and change the syscall. The x86 code handles this (it expects orig_ax to always be the desired syscall). In the ARM case, this means we should not be touching current_thread_info()->syscall after its initial assignment. All failures should result in a -1 syscall, though. Based on patch by Ricky Zhou. Signed-off-by: Kees Cook <keescook@chromium.org> Cc: Ricky Zhou <rickyz@google.com> Cc: stable@vger.kernel.org --- arch/arm/kernel/ptrace.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)