On Mon, Aug 14, 2017 at 2:41 AM, James Hogan <james.hogan@xxxxxxxxxx> wrote: > On Fri, Aug 11, 2017 at 03:23:34PM -0700, Kees Cook wrote: >> On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@xxxxxxxxxx> wrote: >> > Add a PTRACE_SET_SYSCALL ptrace operation to allow the system call to be >> > cancelled independently to the value of the v0 system call number >> > register. >> > >> > This is needed for SECCOMP_RET_TRACE when the tracer wants to cancel the >> > system call, since it has to set both the system call number to -1 and >> > the chosen return value, both of which reside in the same register (v0). >> > The tracer should set the return value first, followed by >> > PTRACE_SET_SYSCALL to set the system call number to -1. >> > >> > That is in contrast to the normal ptrace syscall hook which triggers the >> > tracer on both entry and exit, allowing the system call to be cancelled >> > during the entry hook (setting system call number register to -1, or >> > optionally using PTRACE_SET_SYSCALL), separately to setting the return >> > value during the exit hook. >> > >> > Positive values (to change the syscall that should be executed instead >> > of cancelling it entirely) are explicitly disallowed at the moment. The >> > same thing can be done safely already by writing the v0 system call >> > number register and the argument registers, and allowing >> > thread_info::syscall to be changed to a different value independently of >> > the v0 register would potentially allow seccomp or the syscall trace >> > events to be fooled into thinking a different system call was being >> > executed. >> >> Wouldn't the sycall be reloaded, so no spoofing could occur? > > The case I was thinking of was: > - PTRACE_POKEUSR v0 = __NR_some_disallowed_syscall > - PTRACE_SET_SYSCALL __NR_some_allowed_syscall > > syscall_get_nr() will return __NR_some_allowed_syscall, so seccomp will > allow, but when syscall_trace_enter() returns to syscall_trace_entry in > arch/mips/kernel/scall32-o32.S, it will reload the syscall number from > v0 (i.e. __NR_some_disallowed_syscall). IIUC, the issue is that v0 holds syscall on entry and syscall return on exit. Isn't it possible to rework all the entry logic to examine only thread_info->syscall and ignore v0 during the ptrace and seccomp events? i.e. SET_SYSCALL can modify ti->syscall, and only if it goes to -1 only then will v0 be examined for a result? (If I'm reading scall32-o32.S, I think this means loading the new syscall from thread_info instead of registers after syscall_trace_enter.) If that is possible, it doesn't have to happen in this patch, obviously. Incremental is fine. :) >> Regardless, can you update >> tools/testing/selftests/seccomp/seccomp_bpf.c to update or eliminate >> the MIPS-only SYSCALL_NUM_RET_SHARE_REG special-case? (Or maybe it >> needs to be further special-cased to split syscall-changing from >> syscall-cancelling?) > > Sure, i'll look into that, > > Thanks for reviewing, Sure thing, thanks! -Kees -- Kees Cook Pixel Security
