Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1710067 > unrolled thread
| Started by | Kees Cook <keescook@chromium.org> |
|---|---|
| First post | 2017-08-12 00:30 +0200 |
| Last post | 2017-08-14 20:00 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation Kees Cook <keescook@chromium.org> - 2017-08-12 00:30 +0200
Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation James Hogan <james.hogan@imgtec.com> - 2017-08-14 11:50 +0200
Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation Kees Cook <keescook@chromium.org> - 2017-08-14 20:00 +0200
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-12 00:30 +0200 |
| Subject | Re: [PATCH 4/4] MIPS/ptrace: Add PTRACE_SET_SYSCALL operation |
| Message-ID | <udqBk-Ku-15@gated-at.bofh.it> |
On Fri, Aug 11, 2017 at 1:56 PM, James Hogan <james.hogan@imgtec.com> 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?
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?)
-Kees
>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: linux-mips@linux-mips.org
> ---
> arch/mips/include/uapi/asm/ptrace.h | 1 +
> arch/mips/kernel/ptrace.c | 11 +++++++++++
> arch/mips/kernel/ptrace32.c | 11 +++++++++++
> 3 files changed, 23 insertions(+)
>
> diff --git a/arch/mips/include/uapi/asm/ptrace.h b/arch/mips/include/uapi/asm/ptrace.h
> index 91a3d197ede3..23af103c4e8d 100644
> --- a/arch/mips/include/uapi/asm/ptrace.h
> +++ b/arch/mips/include/uapi/asm/ptrace.h
> @@ -58,6 +58,7 @@ struct pt_regs {
>
> #define PTRACE_GET_THREAD_AREA 25
> #define PTRACE_SET_THREAD_AREA 26
> +#define PTRACE_SET_SYSCALL 27
>
> /* Calls to trace a 64bit program from a 32bit program. */
> #define PTRACE_PEEKTEXT_3264 0xc0
> diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> index 465fc5633e61..9bf31a990c6e 100644
> --- a/arch/mips/kernel/ptrace.c
> +++ b/arch/mips/kernel/ptrace.c
> @@ -853,6 +853,17 @@ long arch_ptrace(struct task_struct *child, long request,
> ret = put_user(task_thread_info(child)->tp_value, datalp);
> break;
>
> + case PTRACE_SET_SYSCALL:
> + /*
> + * This is currently only useful to cancel the syscall from a
> + * seccomp RET_TRACE tracer.
> + */
> + if ((long)data >= 0)
> + return -EINVAL;
> + task_thread_info(child)->syscall = -1;
> + ret = 0;
> + break;
> +
> case PTRACE_GET_WATCH_REGS:
> ret = ptrace_get_watch_regs(child, addrp);
> break;
> diff --git a/arch/mips/kernel/ptrace32.c b/arch/mips/kernel/ptrace32.c
> index 2b9260f92ccd..cca76aec9c10 100644
> --- a/arch/mips/kernel/ptrace32.c
> +++ b/arch/mips/kernel/ptrace32.c
> @@ -287,6 +287,17 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> (unsigned int __user *) (unsigned long) data);
> break;
>
> + case PTRACE_SET_SYSCALL:
> + /*
> + * This is currently only useful to cancel the syscall from a
> + * seccomp RET_TRACE tracer.
> + */
> + if ((long)data >= 0)
> + return -EINVAL;
> + task_thread_info(child)->syscall = -1;
> + ret = 0;
> + break;
> +
> case PTRACE_GET_THREAD_AREA_3264:
> ret = put_user(task_thread_info(child)->tp_value,
> (unsigned long __user *) (unsigned long) data);
> --
> 2.13.2
>
--
Kees Cook
Pixel Security
[toc] | [next] | [standalone]
| From | James Hogan <james.hogan@imgtec.com> |
|---|---|
| Date | 2017-08-14 11:50 +0200 |
| Message-ID | <uekau-2tS-9@gated-at.bofh.it> |
| In reply to | #1710067 |
[Multipart message — attachments visible in raw view] — view raw
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@imgtec.com> 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).
>
> 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,
Cheers
James
>
> -Kees
>
> >
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Will Drewry <wad@chromium.org>
> > Cc: linux-mips@linux-mips.org
> > ---
> > arch/mips/include/uapi/asm/ptrace.h | 1 +
> > arch/mips/kernel/ptrace.c | 11 +++++++++++
> > arch/mips/kernel/ptrace32.c | 11 +++++++++++
> > 3 files changed, 23 insertions(+)
> >
> > diff --git a/arch/mips/include/uapi/asm/ptrace.h b/arch/mips/include/uapi/asm/ptrace.h
> > index 91a3d197ede3..23af103c4e8d 100644
> > --- a/arch/mips/include/uapi/asm/ptrace.h
> > +++ b/arch/mips/include/uapi/asm/ptrace.h
> > @@ -58,6 +58,7 @@ struct pt_regs {
> >
> > #define PTRACE_GET_THREAD_AREA 25
> > #define PTRACE_SET_THREAD_AREA 26
> > +#define PTRACE_SET_SYSCALL 27
> >
> > /* Calls to trace a 64bit program from a 32bit program. */
> > #define PTRACE_PEEKTEXT_3264 0xc0
> > diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
> > index 465fc5633e61..9bf31a990c6e 100644
> > --- a/arch/mips/kernel/ptrace.c
> > +++ b/arch/mips/kernel/ptrace.c
> > @@ -853,6 +853,17 @@ long arch_ptrace(struct task_struct *child, long request,
> > ret = put_user(task_thread_info(child)->tp_value, datalp);
> > break;
> >
> > + case PTRACE_SET_SYSCALL:
> > + /*
> > + * This is currently only useful to cancel the syscall from a
> > + * seccomp RET_TRACE tracer.
> > + */
> > + if ((long)data >= 0)
> > + return -EINVAL;
> > + task_thread_info(child)->syscall = -1;
> > + ret = 0;
> > + break;
> > +
> > case PTRACE_GET_WATCH_REGS:
> > ret = ptrace_get_watch_regs(child, addrp);
> > break;
> > diff --git a/arch/mips/kernel/ptrace32.c b/arch/mips/kernel/ptrace32.c
> > index 2b9260f92ccd..cca76aec9c10 100644
> > --- a/arch/mips/kernel/ptrace32.c
> > +++ b/arch/mips/kernel/ptrace32.c
> > @@ -287,6 +287,17 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> > (unsigned int __user *) (unsigned long) data);
> > break;
> >
> > + case PTRACE_SET_SYSCALL:
> > + /*
> > + * This is currently only useful to cancel the syscall from a
> > + * seccomp RET_TRACE tracer.
> > + */
> > + if ((long)data >= 0)
> > + return -EINVAL;
> > + task_thread_info(child)->syscall = -1;
> > + ret = 0;
> > + break;
> > +
> > case PTRACE_GET_THREAD_AREA_3264:
> > ret = put_user(task_thread_info(child)->tp_value,
> > (unsigned long __user *) (unsigned long) data);
> > --
> > 2.13.2
> >
>
>
>
> --
> Kees Cook
> Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-08-14 20:00 +0200 |
| Message-ID | <uerOG-79Q-33@gated-at.bofh.it> |
| In reply to | #1710760 |
On Mon, Aug 14, 2017 at 2:41 AM, James Hogan <james.hogan@imgtec.com> 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@imgtec.com> 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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web