Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation

[Kees Cook]

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