From: Andy Lutomirski <luto@amacapital.net> Date: 2014-09-05 22:14:08
This applies to:
git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git seccomp-fastpath
Gitweb:
https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=seccomp/fastpath
This is both a cleanup and a speedup. It reduces overhead due to
installing a trivial seccomp filter by 87%. The speedup comes from
avoiding the full syscall tracing mechanism for filters that don't
return SECCOMP_RET_TRACE.
This series depends on splitting the seccomp hooks into two phases.
The first phase evaluates the filter; it can skip syscalls, allow
them, kill the calling task, or pass a u32 to the second phase. The
second phase requires a full tracing context, and it sends ptrace
events if necessary. The seccomp core part is in Kees' seccomp/fastpath
tree.
These patches implement a similar split for the x86 syscall
entry work. The C callback is invoked in two phases: the first has
only a partial frame, and it can request phase 2 processing with a
full frame.
Finally, I switch the 64-bit system_call code to use the new split
entry work. This is a net deletion of assembly code: it replaces
all of the audit entry muck.
In the process, I fixed some bugs.
If this is acceptable, someone can do the same tweak for the
ia32entry and entry_32 code.
This passes all seccomp tests that I know of.
Changes from v4:
- Rebased (which seems to have been a no-op)
- Fixed embarrassing bug that broke allnoconfig
(patch 3 was missing an ifdef).
Changes from v3:
- Dropped the core seccomp changes from the email -- Kees has applied them.
- Add patch 2 (the TIF_NOHZ change).
- Fix TIF_NOHZ in the two-phase entry code (thanks, Oleg).
Changes from v2:
- Fixed 32-bit x86 build (and the tests pass).
- Put the doc patch where it belongs.
Changes from v1:
- Rebased on top of Kees' shiny new seccomp tree (no effect on the x86
part).
- Improved patch 6 vs patch 7 split (thanks Alexei!)
- Fixed bogus -ENOSYS in patch 5 (thanks Kees!)
- Improved changelog message in patch 6.
Changes from RFC version:
- The first three patches are more or less the same
- The rest is more or less a rewrite
Andy Lutomirski (5):
x86,x32,audit: Fix x32's AUDIT_ARCH wrt audit
x86,entry: Only call user_exit if TIF_NOHZ
x86: Split syscall_trace_enter into two phases
x86_64,entry: Treat regs->ax the same in fastpath and slowpath
syscalls
x86_64,entry: Use split-phase syscall_trace_enter for 64-bit syscalls
arch/x86/include/asm/calling.h | 6 +-
arch/x86/include/asm/ptrace.h | 5 ++
arch/x86/kernel/entry_64.S | 51 +++++--------
arch/x86/kernel/ptrace.c | 165 +++++++++++++++++++++++++++++++++--------
4 files changed, 164 insertions(+), 63 deletions(-)
--
1.9.3
From: Andy Lutomirski <luto@amacapital.net> Date: 2014-09-05 22:14:14
is_compat_task() is the wrong check for audit arch; the check should
be is_ia32_task(): x32 syscalls should be AUDIT_ARCH_X86_64, not
AUDIT_ARCH_I386.
CONFIG_AUDITSYSCALL is currently incompatible with x32, so this has
no visible effect.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/kernel/ptrace.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
From: Andy Lutomirski <luto@amacapital.net> Date: 2014-09-05 22:14:18
The RCU context tracking code requires that arch code call
user_exit() on any entry into kernel code if TIF_NOHZ is set. This
patch adds a check for TIF_NOHZ and a comment to the syscall entry
tracing code.
The main purpose of this patch is to make the code easier to follow:
one can read the body of user_exit and of every function it calls
without finding any explanation of why it's called for traced
syscalls but not for untraced syscalls. This makes it clear when
user_exit() is necessary.
Cc: Frederic Weisbecker <redacted>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/kernel/ptrace.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Andy Lutomirski <luto@amacapital.net> Date: 2014-09-05 22:14:23
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
The intent is that phase 1 can be called from the syscall fast path.
In this implementation, phase1 can handle any combination of
TIF_NOHZ (RCU context tracking), TIF_SECCOMP, and TIF_SYSCALL_AUDIT,
unless seccomp requests a ptrace event, in which case phase2 is
forced.
In principle, this could yield a big speedup for TIF_NOHZ as well as
for TIF_SECCOMP if syscall exit work were similarly split up.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/include/asm/ptrace.h | 5 ++
arch/x86/kernel/ptrace.c | 157 +++++++++++++++++++++++++++++++++++-------
2 files changed, 138 insertions(+), 24 deletions(-)
@@ -1441,20 +1441,126 @@ void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,force_sig_info(SIGTRAP,&info,tsk);}+staticvoiddo_audit_syscall_entry(structpt_regs*regs,u32arch)+{+#ifdef CONFIG_X86_64+if(arch==AUDIT_ARCH_X86_64){+audit_syscall_entry(arch,regs->orig_ax,regs->di,+regs->si,regs->dx,regs->r10);+}else+#endif+{+audit_syscall_entry(arch,regs->orig_ax,regs->bx,+regs->cx,regs->dx,regs->si);+}+}+/*-*Wemustreturnthesyscallnumbertoactuallylookupinthetable.-*Thiscanbe-1Ltoskiprunninganysyscallatall.+*Wecanreturn0toresumethesyscalloranythingelsetogotophase+*2.Ifweresumethesyscall,weneedtoputsomethingappropriatein+*regs->orig_ax.+*+*NB:Wedon'thavefullpt_regshere,butregs->orig_axandregs->ax+*arefullyfunctional.+*+*Forphase2'sbenefit,ourreturnvalueis:+*0:resumethesyscall+*1:gotophase2;noseccompphase2needed+*anythingelse:gotophase2;passreturnvaluetoseccomp*/-longsyscall_trace_enter(structpt_regs*regs)+unsignedlongsyscall_trace_enter_phase1(structpt_regs*regs,u32arch){-longret=0;+unsignedlongret=0;+u32work;++BUG_ON(regs!=task_pt_regs(current));++work=ACCESS_ONCE(current_thread_info()->flags)&+_TIF_WORK_SYSCALL_ENTRY;/**IfTIF_NOHZisset,wearerequiredtocalluser_exit()before*doinganythingthatcouldtouchRCU.*/-if(test_thread_flag(TIF_NOHZ))+if(work&_TIF_NOHZ){user_exit();+work&=~TIF_NOHZ;+}++#ifdef CONFIG_SECCOMP+/*+*Doseccompfirst--itshouldminimizeexposureofother+*code,andkeepingseccompfastisprobablymorevaluable+*thantherestofthis.+*/+if(work&_TIF_SECCOMP){+structseccomp_datasd;++sd.arch=arch;+sd.nr=regs->orig_ax;+sd.instruction_pointer=regs->ip;+#ifdef CONFIG_X86_64+if(arch==AUDIT_ARCH_X86_64){+sd.args[0]=regs->di;+sd.args[1]=regs->si;+sd.args[2]=regs->dx;+sd.args[3]=regs->r10;+sd.args[4]=regs->r8;+sd.args[5]=regs->r9;+}else+#endif+{+sd.args[0]=regs->bx;+sd.args[1]=regs->cx;+sd.args[2]=regs->dx;+sd.args[3]=regs->si;+sd.args[4]=regs->di;+sd.args[5]=regs->bp;+}++BUILD_BUG_ON(SECCOMP_PHASE1_OK!=0);+BUILD_BUG_ON(SECCOMP_PHASE1_SKIP!=1);++ret=seccomp_phase1(&sd);+if(ret==SECCOMP_PHASE1_SKIP){+regs->orig_ax=-1;+ret=0;+}elseif(ret!=SECCOMP_PHASE1_OK){+returnret;/* Go directly to phase 2 */+}++work&=~_TIF_SECCOMP;+}+#endif++/* Do our best to finish without phase 2. */+if(work==0)+returnret;/* seccomp and/or nohz only (ret == 0 here) */++#ifdef CONFIG_AUDITSYSCALL+if(work==_TIF_SYSCALL_AUDIT){+/*+*Ifthereisnomoreworktobedoneexceptauditing,+*thenauditinphase1.Phase2alwaysaudits,so,if+*weaudithere,thenwecan'tgoontophase2.+*/+do_audit_syscall_entry(regs,arch);+return0;+}+#endif++return1;/* Something is enabled that we can't handle in phase 1 */+}++/* Returns the syscall nr to run (which should match regs->orig_ax). */+longsyscall_trace_enter_phase2(structpt_regs*regs,u32arch,+unsignedlongphase1_result)+{+longret=0;+u32work=ACCESS_ONCE(current_thread_info()->flags)&+_TIF_WORK_SYSCALL_ENTRY;++BUG_ON(regs!=task_pt_regs(current));/**Ifwesteppedintoasysenter/syscallinsn,ittrappedin
@@ -1463,17 +1569,21 @@ long syscall_trace_enter(struct pt_regs *regs)*do_debug()andweneedtosetitagaintorestoretheuser*state.Ifweenteredontheslowpath,TFwasalreadyset.*/-if(test_thread_flag(TIF_SINGLESTEP))+if(work&_TIF_SINGLESTEP)regs->flags|=X86_EFLAGS_TF;-/* do the secure computing check first */-if(secure_computing()){+#ifdef CONFIG_SECCOMP+/*+*Callseccomp_phase2beforerunningtheotherhookssothat+*theycanseeanychangesmadebyaseccomptracer.+*/+if(phase1_result>1&&seccomp_phase2(phase1_result)){/* seccomp failures shouldn't expose any additional code. */-ret=-1L;-gotoout;+return-1;}+#endif-if(unlikely(test_thread_flag(TIF_SYSCALL_EMU)))+if(unlikely(work&_TIF_SYSCALL_EMU))ret=-1L;if((ret||test_thread_flag(TIF_SYSCALL_TRACE))&&
@@ -1483,23 +1593,22 @@ long syscall_trace_enter(struct pt_regs *regs)if(unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))trace_sys_enter(regs,regs->orig_ax);-if(is_ia32_task())-audit_syscall_entry(AUDIT_ARCH_I386,-regs->orig_ax,-regs->bx,regs->cx,-regs->dx,regs->si);-#ifdef CONFIG_X86_64-else-audit_syscall_entry(AUDIT_ARCH_X86_64,-regs->orig_ax,-regs->di,regs->si,-regs->dx,regs->r10);-#endif+do_audit_syscall_entry(regs,arch);-out:returnret?:regs->orig_ax;}+longsyscall_trace_enter(structpt_regs*regs)+{+u32arch=is_ia32_task()?AUDIT_ARCH_I386:AUDIT_ARCH_X86_64;+unsignedlongphase1_result=syscall_trace_enter_phase1(regs,arch);++if(phase1_result==0)+returnregs->orig_ax;+else+returnsyscall_trace_enter_phase2(regs,arch,phase1_result);+}+voidsyscall_trace_leave(structpt_regs*regs){boolstep;
From: Dmitry V. Levin <hidden> Date: 2015-02-05 21:29:22
Hi,
On Fri, Sep 05, 2014 at 03:13:54PM -0700, Andy Lutomirski wrote:
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
This breaks ptrace, see below.
quoted hunk
The intent is that phase 1 can be called from the syscall fast path.
In this implementation, phase1 can handle any combination of
TIF_NOHZ (RCU context tracking), TIF_SECCOMP, and TIF_SYSCALL_AUDIT,
unless seccomp requests a ptrace event, in which case phase2 is
forced.
In principle, this could yield a big speedup for TIF_NOHZ as well as
for TIF_SECCOMP if syscall exit work were similarly split up.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/include/asm/ptrace.h | 5 ++
arch/x86/kernel/ptrace.c | 157 +++++++++++++++++++++++++++++++++++-------
2 files changed, 138 insertions(+), 24 deletions(-)
On Thu, Feb 5, 2015 at 1:19 PM, Dmitry V. Levin [off-list ref] wrote:
Hi,
On Fri, Sep 05, 2014 at 03:13:54PM -0700, Andy Lutomirski wrote:
quoted
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
This breaks ptrace, see below.
quoted
The intent is that phase 1 can be called from the syscall fast path.
In this implementation, phase1 can handle any combination of
TIF_NOHZ (RCU context tracking), TIF_SECCOMP, and TIF_SYSCALL_AUDIT,
unless seccomp requests a ptrace event, in which case phase2 is
forced.
In principle, this could yield a big speedup for TIF_NOHZ as well as
for TIF_SECCOMP if syscall exit work were similarly split up.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/include/asm/ptrace.h | 5 ++
arch/x86/kernel/ptrace.c | 157 +++++++++++++++++++++++++++++++++++-------
2 files changed, 138 insertions(+), 24 deletions(-)
How the tracer is expected to get the correct syscall number after that?
There shouldn't be a tracer if a skip is encountered. (A seccomp skip
would skip ptrace.) This behavior hasn't changed, but maybe I don't
see what you mean? (I haven't encountered any problems with syscall
tracing as a result of these changes.)
-Kees
--
Kees Cook
Chrome OS Security
From: Dmitry V. Levin <hidden> Date: 2015-02-05 21:40:32
On Thu, Feb 05, 2015 at 01:27:16PM -0800, Kees Cook wrote:
On Thu, Feb 5, 2015 at 1:19 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
Hi,
On Fri, Sep 05, 2014 at 03:13:54PM -0700, Andy Lutomirski wrote:
quoted
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
This breaks ptrace, see below.
quoted
The intent is that phase 1 can be called from the syscall fast path.
In this implementation, phase1 can handle any combination of
TIF_NOHZ (RCU context tracking), TIF_SECCOMP, and TIF_SYSCALL_AUDIT,
unless seccomp requests a ptrace event, in which case phase2 is
forced.
In principle, this could yield a big speedup for TIF_NOHZ as well as
for TIF_SECCOMP if syscall exit work were similarly split up.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/include/asm/ptrace.h | 5 ++
arch/x86/kernel/ptrace.c | 157 +++++++++++++++++++++++++++++++++++-------
2 files changed, 138 insertions(+), 24 deletions(-)
How the tracer is expected to get the correct syscall number after that?
There shouldn't be a tracer if a skip is encountered. (A seccomp skip
would skip ptrace.) This behavior hasn't changed, but maybe I don't
see what you mean? (I haven't encountered any problems with syscall
tracing as a result of these changes.)
SECCOMP_RET_ERRNO leads to SECCOMP_PHASE1_SKIP, and if there is a tracer,
it will get -1 as a syscall number.
I've found this while testing a strace parser for
SECCOMP_MODE_FILTER/SECCOMP_SET_MODE_FILTER, so the problem is quite real.
--
ldv
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-02-05 21:53:04
On Thu, Feb 5, 2015 at 1:40 PM, Dmitry V. Levin [off-list ref] wrote:
On Thu, Feb 05, 2015 at 01:27:16PM -0800, Kees Cook wrote:
quoted
On Thu, Feb 5, 2015 at 1:19 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
Hi,
On Fri, Sep 05, 2014 at 03:13:54PM -0700, Andy Lutomirski wrote:
quoted
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
This breaks ptrace, see below.
quoted
The intent is that phase 1 can be called from the syscall fast path.
In this implementation, phase1 can handle any combination of
TIF_NOHZ (RCU context tracking), TIF_SECCOMP, and TIF_SYSCALL_AUDIT,
unless seccomp requests a ptrace event, in which case phase2 is
forced.
In principle, this could yield a big speedup for TIF_NOHZ as well as
for TIF_SECCOMP if syscall exit work were similarly split up.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/include/asm/ptrace.h | 5 ++
arch/x86/kernel/ptrace.c | 157 +++++++++++++++++++++++++++++++++++-------
2 files changed, 138 insertions(+), 24 deletions(-)
How the tracer is expected to get the correct syscall number after that?
There shouldn't be a tracer if a skip is encountered. (A seccomp skip
would skip ptrace.) This behavior hasn't changed, but maybe I don't
see what you mean? (I haven't encountered any problems with syscall
tracing as a result of these changes.)
SECCOMP_RET_ERRNO leads to SECCOMP_PHASE1_SKIP, and if there is a tracer,
it will get -1 as a syscall number.
I've found this while testing a strace parser for
SECCOMP_MODE_FILTER/SECCOMP_SET_MODE_FILTER, so the problem is quite real.
Hasn't it always been this way?
I admit that I kind of wish this worked the other way -- that is, I
think it would be nice to have a mode in which ptrace runs before
seccomp, which would close the ptrace hole (where ptrace can do things
that seccomp would disallow) and maybe have more comprehensible
results.
--Andy
On Thu, Feb 5, 2015 at 1:52 PM, Andy Lutomirski [off-list ref] wrote:
On Thu, Feb 5, 2015 at 1:40 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Thu, Feb 05, 2015 at 01:27:16PM -0800, Kees Cook wrote:
quoted
On Thu, Feb 5, 2015 at 1:19 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
Hi,
On Fri, Sep 05, 2014 at 03:13:54PM -0700, Andy Lutomirski wrote:
quoted
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
This breaks ptrace, see below.
quoted
The intent is that phase 1 can be called from the syscall fast path.
In this implementation, phase1 can handle any combination of
TIF_NOHZ (RCU context tracking), TIF_SECCOMP, and TIF_SYSCALL_AUDIT,
unless seccomp requests a ptrace event, in which case phase2 is
forced.
In principle, this could yield a big speedup for TIF_NOHZ as well as
for TIF_SECCOMP if syscall exit work were similarly split up.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/include/asm/ptrace.h | 5 ++
arch/x86/kernel/ptrace.c | 157 +++++++++++++++++++++++++++++++++++-------
2 files changed, 138 insertions(+), 24 deletions(-)
How the tracer is expected to get the correct syscall number after that?
There shouldn't be a tracer if a skip is encountered. (A seccomp skip
would skip ptrace.) This behavior hasn't changed, but maybe I don't
see what you mean? (I haven't encountered any problems with syscall
tracing as a result of these changes.)
SECCOMP_RET_ERRNO leads to SECCOMP_PHASE1_SKIP, and if there is a tracer,
it will get -1 as a syscall number.
I've found this while testing a strace parser for
SECCOMP_MODE_FILTER/SECCOMP_SET_MODE_FILTER, so the problem is quite real.
Hasn't it always been this way?
As far as I know, yes, it's always been this way. The point is to the
skip the syscall, which is what the -1 signals. Userspace then reads
back the errno.
I admit that I kind of wish this worked the other way -- that is, I
think it would be nice to have a mode in which ptrace runs before
seccomp, which would close the ptrace hole (where ptrace can do things
that seccomp would disallow) and maybe have more comprehensible
results.
I prefer keeping the seccomp attack surface as tiny as possible. I
would not like to ptrace happening before seccomp.
-Kees
--
Kees Cook
Chrome OS Security
From: Dmitry V. Levin <hidden> Date: 2015-02-05 23:39:49
On Thu, Feb 05, 2015 at 03:12:39PM -0800, Kees Cook wrote:
On Thu, Feb 5, 2015 at 1:52 PM, Andy Lutomirski [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 1:40 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Thu, Feb 05, 2015 at 01:27:16PM -0800, Kees Cook wrote:
quoted
On Thu, Feb 5, 2015 at 1:19 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
Hi,
On Fri, Sep 05, 2014 at 03:13:54PM -0700, Andy Lutomirski wrote:
quoted
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
This breaks ptrace, see below.
[...]
quoted
quoted
quoted
quoted
quoted
+ ret = seccomp_phase1(&sd);
+ if (ret == SECCOMP_PHASE1_SKIP) {
+ regs->orig_ax = -1;
How the tracer is expected to get the correct syscall number after that?
There shouldn't be a tracer if a skip is encountered. (A seccomp skip
would skip ptrace.) This behavior hasn't changed, but maybe I don't
see what you mean? (I haven't encountered any problems with syscall
tracing as a result of these changes.)
SECCOMP_RET_ERRNO leads to SECCOMP_PHASE1_SKIP, and if there is a tracer,
it will get -1 as a syscall number.
I've found this while testing a strace parser for
SECCOMP_MODE_FILTER/SECCOMP_SET_MODE_FILTER, so the problem is quite real.
Hasn't it always been this way?
As far as I know, yes, it's always been this way. The point is to the
skip the syscall, which is what the -1 signals. Userspace then reads
back the errno.
There is a clear difference: before these changes, SECCOMP_RET_ERRNO used
to keep the syscall number unchanged and suppress syscall-exit-stop event,
which was awful because userspace cannot distinguish syscall-enter-stop
from syscall-exit-stop and therefore relies on the kernel that
syscall-enter-stop is followed by syscall-exit-stop (or tracee's death, etc.).
After these changes, SECCOMP_RET_ERRNO no longer causes syscall-exit-stop
events to be suppressed, but now the syscall number is lost.
--
ldv
On Thu, Feb 5, 2015 at 3:39 PM, Dmitry V. Levin [off-list ref] wrote:
On Thu, Feb 05, 2015 at 03:12:39PM -0800, Kees Cook wrote:
quoted
On Thu, Feb 5, 2015 at 1:52 PM, Andy Lutomirski [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 1:40 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Thu, Feb 05, 2015 at 01:27:16PM -0800, Kees Cook wrote:
quoted
On Thu, Feb 5, 2015 at 1:19 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
Hi,
On Fri, Sep 05, 2014 at 03:13:54PM -0700, Andy Lutomirski wrote:
quoted
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
This breaks ptrace, see below.
[...]
quoted
quoted
quoted
quoted
quoted
quoted
+ ret = seccomp_phase1(&sd);
+ if (ret == SECCOMP_PHASE1_SKIP) {
+ regs->orig_ax = -1;
How the tracer is expected to get the correct syscall number after that?
There shouldn't be a tracer if a skip is encountered. (A seccomp skip
would skip ptrace.) This behavior hasn't changed, but maybe I don't
see what you mean? (I haven't encountered any problems with syscall
tracing as a result of these changes.)
SECCOMP_RET_ERRNO leads to SECCOMP_PHASE1_SKIP, and if there is a tracer,
it will get -1 as a syscall number.
I've found this while testing a strace parser for
SECCOMP_MODE_FILTER/SECCOMP_SET_MODE_FILTER, so the problem is quite real.
Hasn't it always been this way?
As far as I know, yes, it's always been this way. The point is to the
skip the syscall, which is what the -1 signals. Userspace then reads
back the errno.
There is a clear difference: before these changes, SECCOMP_RET_ERRNO used
to keep the syscall number unchanged and suppress syscall-exit-stop event,
which was awful because userspace cannot distinguish syscall-enter-stop
from syscall-exit-stop and therefore relies on the kernel that
syscall-enter-stop is followed by syscall-exit-stop (or tracee's death, etc.).
After these changes, SECCOMP_RET_ERRNO no longer causes syscall-exit-stop
events to be suppressed, but now the syscall number is lost.
Ah-ha! Okay, thanks, I understand now. I think this means seccomp
phase1 should not treat RET_ERRNO as a "skip" event. Andy, what do you
think here?
-Kees
--
Kees Cook
Chrome OS Security
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-02-06 00:09:30
On Thu, Feb 5, 2015 at 3:49 PM, Kees Cook [off-list ref] wrote:
On Thu, Feb 5, 2015 at 3:39 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Thu, Feb 05, 2015 at 03:12:39PM -0800, Kees Cook wrote:
quoted
On Thu, Feb 5, 2015 at 1:52 PM, Andy Lutomirski [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 1:40 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Thu, Feb 05, 2015 at 01:27:16PM -0800, Kees Cook wrote:
quoted
On Thu, Feb 5, 2015 at 1:19 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
Hi,
On Fri, Sep 05, 2014 at 03:13:54PM -0700, Andy Lutomirski wrote:
quoted
This splits syscall_trace_enter into syscall_trace_enter_phase1 and
syscall_trace_enter_phase2. Only phase 2 has full pt_regs, and only
phase 2 is permitted to modify any of pt_regs except for orig_ax.
This breaks ptrace, see below.
[...]
quoted
quoted
quoted
quoted
quoted
quoted
+ ret = seccomp_phase1(&sd);
+ if (ret == SECCOMP_PHASE1_SKIP) {
+ regs->orig_ax = -1;
How the tracer is expected to get the correct syscall number after that?
There shouldn't be a tracer if a skip is encountered. (A seccomp skip
would skip ptrace.) This behavior hasn't changed, but maybe I don't
see what you mean? (I haven't encountered any problems with syscall
tracing as a result of these changes.)
SECCOMP_RET_ERRNO leads to SECCOMP_PHASE1_SKIP, and if there is a tracer,
it will get -1 as a syscall number.
I've found this while testing a strace parser for
SECCOMP_MODE_FILTER/SECCOMP_SET_MODE_FILTER, so the problem is quite real.
Hasn't it always been this way?
As far as I know, yes, it's always been this way. The point is to the
skip the syscall, which is what the -1 signals. Userspace then reads
back the errno.
There is a clear difference: before these changes, SECCOMP_RET_ERRNO used
to keep the syscall number unchanged and suppress syscall-exit-stop event,
which was awful because userspace cannot distinguish syscall-enter-stop
from syscall-exit-stop and therefore relies on the kernel that
syscall-enter-stop is followed by syscall-exit-stop (or tracee's death, etc.).
After these changes, SECCOMP_RET_ERRNO no longer causes syscall-exit-stop
events to be suppressed, but now the syscall number is lost.
Ah-ha! Okay, thanks, I understand now. I think this means seccomp
phase1 should not treat RET_ERRNO as a "skip" event. Andy, what do you
think here?
I still don't quite see how this change caused this. I can play with
it a bit more. But RET_ERRNO *has* to be some kind of skip event,
because it needs to skip the syscall.
We could change this by treating RET_ERRNO as an instruction to enter
phase 2 and then asking for a skip in phase 2 without changing
orig_ax, but IMO this is pretty ugly.
I think this all kind of sucks. We're trying to run ptrace after
seccomp, so ptrace is seeing the syscalls as transformed by seccomp.
That means that if we use RET_TRAP, then ptrace will see the
possibly-modified syscall, if we use RET_ERRNO, then ptrace is (IMO
correctly given the current design) showing syscall -1, and if we use
RET_KILL, then ptrace just sees the process mysteriously die.
I think it would be more useful and easier to understand if ptrace saw
syscalls as the traced process saw them, i.e. before seccomp
modification. How would this meaningfully increase the attack
surface? As far as ptrace is concerned, a syscall is just seven
numbers, and as long as a process can issue *any* syscall that seccomp
allows, then it can invoke the ptrace hooks. I don't think the
entry/exit hooks care *at all* about the syscall nr or args.
Audit is a different story. I think we should absolutely continue to
audit syscalls that actually happen, not syscalls that were requested.
Given this bug, I doubt we'd break anything if we changed it, since it
appears that it's already rather broken. Also, changing it would make
me happy, because I want to add a SECCOMP_RET_MONITOR that freezes the
process, sends an event to a seccompfd, and then executes syscalls as
requested by the holder of the seccompfd. Those syscalls would, in
turn, be filtered again through inner layers of seccomp. One sticking
point would be that the current ptrace behavior is very hard to
reconcile with this type of feature.
--Andy
From: Dmitry V. Levin <hidden> Date: 2015-02-06 02:32:56
On Thu, Feb 05, 2015 at 04:09:06PM -0800, Andy Lutomirski wrote:
On Thu, Feb 5, 2015 at 3:49 PM, Kees Cook [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 3:39 PM, Dmitry V. Levin [off-list ref] wrote:
[...]
quoted
quoted
There is a clear difference: before these changes, SECCOMP_RET_ERRNO used
to keep the syscall number unchanged and suppress syscall-exit-stop event,
which was awful because userspace cannot distinguish syscall-enter-stop
from syscall-exit-stop and therefore relies on the kernel that
syscall-enter-stop is followed by syscall-exit-stop (or tracee's death, etc.).
After these changes, SECCOMP_RET_ERRNO no longer causes syscall-exit-stop
events to be suppressed, but now the syscall number is lost.
Ah-ha! Okay, thanks, I understand now. I think this means seccomp
phase1 should not treat RET_ERRNO as a "skip" event. Andy, what do you
think here?
I still don't quite see how this change caused this.
I can play with
it a bit more. But RET_ERRNO *has* to be some kind of skip event,
because it needs to skip the syscall.
We could change this by treating RET_ERRNO as an instruction to enter
phase 2 and then asking for a skip in phase 2 without changing
orig_ax, but IMO this is pretty ugly.
I think this all kind of sucks. We're trying to run ptrace after
seccomp, so ptrace is seeing the syscalls as transformed by seccomp.
That means that if we use RET_TRAP, then ptrace will see the
possibly-modified syscall, if we use RET_ERRNO, then ptrace is (IMO
correctly given the current design) showing syscall -1, and if we use
RET_KILL, then ptrace just sees the process mysteriously die.
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-02-06 02:39:15
On Thu, Feb 5, 2015 at 6:32 PM, Dmitry V. Levin [off-list ref] wrote:
On Thu, Feb 05, 2015 at 04:09:06PM -0800, Andy Lutomirski wrote:
quoted
On Thu, Feb 5, 2015 at 3:49 PM, Kees Cook [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 3:39 PM, Dmitry V. Levin [off-list ref] wrote:
[...]
quoted
quoted
quoted
There is a clear difference: before these changes, SECCOMP_RET_ERRNO used
to keep the syscall number unchanged and suppress syscall-exit-stop event,
which was awful because userspace cannot distinguish syscall-enter-stop
from syscall-exit-stop and therefore relies on the kernel that
syscall-enter-stop is followed by syscall-exit-stop (or tracee's death, etc.).
After these changes, SECCOMP_RET_ERRNO no longer causes syscall-exit-stop
events to be suppressed, but now the syscall number is lost.
Ah-ha! Okay, thanks, I understand now. I think this means seccomp
phase1 should not treat RET_ERRNO as a "skip" event. Andy, what do you
think here?
I still don't quite see how this change caused this.
I can play with
it a bit more. But RET_ERRNO *has* to be some kind of skip event,
because it needs to skip the syscall.
We could change this by treating RET_ERRNO as an instruction to enter
phase 2 and then asking for a skip in phase 2 without changing
orig_ax, but IMO this is pretty ugly.
I think this all kind of sucks. We're trying to run ptrace after
seccomp, so ptrace is seeing the syscalls as transformed by seccomp.
That means that if we use RET_TRAP, then ptrace will see the
possibly-modified syscall, if we use RET_ERRNO, then ptrace is (IMO
correctly given the current design) showing syscall -1, and if we use
RET_KILL, then ptrace just sees the process mysteriously die.
I think this is solidly in the "don't do that" category. Seccomp lets
you tamper with syscalls. If you tamper wrong, then you lose.
Kees, what do you think about reversing the whole thing so that
ptracers always see the original syscall?
--Andy
On Thu, Feb 5, 2015 at 6:38 PM, Andy Lutomirski [off-list ref] wrote:
On Thu, Feb 5, 2015 at 6:32 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Thu, Feb 05, 2015 at 04:09:06PM -0800, Andy Lutomirski wrote:
quoted
On Thu, Feb 5, 2015 at 3:49 PM, Kees Cook [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 3:39 PM, Dmitry V. Levin [off-list ref] wrote:
[...]
quoted
quoted
quoted
There is a clear difference: before these changes, SECCOMP_RET_ERRNO used
to keep the syscall number unchanged and suppress syscall-exit-stop event,
which was awful because userspace cannot distinguish syscall-enter-stop
from syscall-exit-stop and therefore relies on the kernel that
syscall-enter-stop is followed by syscall-exit-stop (or tracee's death, etc.).
After these changes, SECCOMP_RET_ERRNO no longer causes syscall-exit-stop
events to be suppressed, but now the syscall number is lost.
Ah-ha! Okay, thanks, I understand now. I think this means seccomp
phase1 should not treat RET_ERRNO as a "skip" event. Andy, what do you
think here?
I still don't quite see how this change caused this.
I can play with
it a bit more. But RET_ERRNO *has* to be some kind of skip event,
because it needs to skip the syscall.
We could change this by treating RET_ERRNO as an instruction to enter
phase 2 and then asking for a skip in phase 2 without changing
orig_ax, but IMO this is pretty ugly.
I think this all kind of sucks. We're trying to run ptrace after
seccomp, so ptrace is seeing the syscalls as transformed by seccomp.
That means that if we use RET_TRAP, then ptrace will see the
possibly-modified syscall, if we use RET_ERRNO, then ptrace is (IMO
correctly given the current design) showing syscall -1, and if we use
RET_KILL, then ptrace just sees the process mysteriously die.
To save others the link reading: "Linus said he will make sure the no
syscall returns a value in -1 .. -4095 as a valid result so we can
savely test with -4095."
Strictly speaking (ISO C, "man 3 errno"), errno is supposed to be a
full int, though digging around I find this in include/linux/err.h:
/*
* Kernel pointers have redundant information, so we can use a
* scheme where we can return either an error code or a normal
* pointer with the same return value.
*
* This should be a per-architecture thing, to allow different
* error and pointer decisions.
*/
#define MAX_ERRNO 4095
#ifndef __ASSEMBLY__
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
But no architecture overrides this.
quoted
If it isn't too late, I'd recommend changing SECCOMP_RET_DATA mask
applied in SECCOMP_RET_ERRNO case from current 0xffff to 0xfff.
I'm not opposed to this. I would want to more explicitly document the
4095 max value in man pages, though.
I think this is solidly in the "don't do that" category. Seccomp lets
you tamper with syscalls. If you tamper wrong, then you lose.
Kees, what do you think about reversing the whole thing so that
ptracers always see the original syscall?
What do you mean by "reversing"? The interactions I see here are:
PTRACE_SYSCALL
SECCOMP_RET_ERRNO
SECCOMP_RET_TRACE
SECCOMP_RET_TRAP
Both ptrace and seccomp will trigger via _TIF_WORK_SYSCALL_ENTRY. Only
ptrace will trigger via _TIF_WORK_SYSCALL_EXIT.
For SECCOMP_RET_ERRNO to work, we must skip the syscall, as mentioned earlier:
arch/x86/kernel/entry_32.S ...
syscall_trace_entry:
movl $-ENOSYS,PT_EAX(%esp)
movl %esp, %eax
call syscall_trace_enter
/* What it returned is what we'll actually use. */
cmpl $(NR_syscalls), %eax
jnae syscall_call
jmp syscall_exit
END(syscall_trace_entry)
Both before and after the 2-phase change, syscall_trace_enter would
return -1 if it hit SECCOMP_RET_ERRNO, before calling
tracehook_report_syscall_entry. On exit, if PTRACE_SYSCALL, we'd hit
tracehook_report_syscall_exit during syscall_trace_leave, which means
a ptracer will see a syscall-exit-stop without a matching
syscall-enter-stop.
Using SECCOMP_RET_TRACE with PTRACE_SYSCALL in place seems totally
crazy, as the ptracer would need to be the same program, and if it
chose to skip a syscall, it would be in the same place: it would see
PTRACE_EVENT_SECCOMP, then no syscall-enter-stop, then a
syscall-exit-stop. I think we can ignore this pathological case.
Using SECCOMP_RET_TRAP with PTRACE_SYSCALL also results in a skip,
which produces the same "only syscall-exit-stop seen" problem.
In the SECCOMP_RET_ERRNO case, the syscall nr doesn't change (and
isn't executed). In the SECCOMP_RET_TRAP case, the syscall nr doesn't
change (and isn't executed). In the SECCOMP_RET_TRACE, the syscall nr
_could_ change, but the ptracer would be doing it, so the crazy
situation around PTRACE_SYSCALL is probably safe to ignore (as long as
we document what is expected to happen).
So, the question is: should PTRACE_SYSCALL see a syscall that is _not_
being executed (due to seccomp)? Audit doesn't see it currently, and
neither does ptrace. I would argue that it should continue to not see
the syscall. That said, if it shouldn't be shown, we also shouldn't
trigger syscall-exit-stop. If you can convince me it should see
syscall-enter-stop, then I have two questions:
1) Do we accept that a ptracer can interfere with SECCOMP_RET_ERRNO? I
think we probably must, since it can already interfere via
syscall-exit-stop and change the errno. And especially since a ptracer
can change syscalls during syscall-enter-stop to any syscall it wants,
bypassing seccomp. This condition is already documented.
2) What do we do with audit? Suddenly we have ptrace seeing a syscall
that audit doesn't?
And an unrelated thought:
3) Can't we find some way to fix the inability of a ptracer to
distinguish between syscall-enter-stop and syscall-exit-stop?
-Kees
--
Kees Cook
Chrome OS Security
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-02-06 19:32:33
On Fri, Feb 6, 2015 at 11:23 AM, Kees Cook [off-list ref] wrote:
On Thu, Feb 5, 2015 at 6:38 PM, Andy Lutomirski [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 6:32 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Thu, Feb 05, 2015 at 04:09:06PM -0800, Andy Lutomirski wrote:
quoted
On Thu, Feb 5, 2015 at 3:49 PM, Kees Cook [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 3:39 PM, Dmitry V. Levin [off-list ref] wrote:
[...]
quoted
quoted
quoted
There is a clear difference: before these changes, SECCOMP_RET_ERRNO used
to keep the syscall number unchanged and suppress syscall-exit-stop event,
which was awful because userspace cannot distinguish syscall-enter-stop
from syscall-exit-stop and therefore relies on the kernel that
syscall-enter-stop is followed by syscall-exit-stop (or tracee's death, etc.).
After these changes, SECCOMP_RET_ERRNO no longer causes syscall-exit-stop
events to be suppressed, but now the syscall number is lost.
Ah-ha! Okay, thanks, I understand now. I think this means seccomp
phase1 should not treat RET_ERRNO as a "skip" event. Andy, what do you
think here?
I still don't quite see how this change caused this.
I can play with
it a bit more. But RET_ERRNO *has* to be some kind of skip event,
because it needs to skip the syscall.
We could change this by treating RET_ERRNO as an instruction to enter
phase 2 and then asking for a skip in phase 2 without changing
orig_ax, but IMO this is pretty ugly.
I think this all kind of sucks. We're trying to run ptrace after
seccomp, so ptrace is seeing the syscalls as transformed by seccomp.
That means that if we use RET_TRAP, then ptrace will see the
possibly-modified syscall, if we use RET_ERRNO, then ptrace is (IMO
correctly given the current design) showing syscall -1, and if we use
RET_KILL, then ptrace just sees the process mysteriously die.
To save others the link reading: "Linus said he will make sure the no
syscall returns a value in -1 .. -4095 as a valid result so we can
savely test with -4095."
Strictly speaking (ISO C, "man 3 errno"), errno is supposed to be a
full int, though digging around I find this in include/linux/err.h:
/*
* Kernel pointers have redundant information, so we can use a
* scheme where we can return either an error code or a normal
* pointer with the same return value.
*
* This should be a per-architecture thing, to allow different
* error and pointer decisions.
*/
#define MAX_ERRNO 4095
#ifndef __ASSEMBLY__
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
But no architecture overrides this.
quoted
quoted
If it isn't too late, I'd recommend changing SECCOMP_RET_DATA mask
applied in SECCOMP_RET_ERRNO case from current 0xffff to 0xfff.
I'm not opposed to this. I would want to more explicitly document the
4095 max value in man pages, though.
quoted
I think this is solidly in the "don't do that" category. Seccomp lets
you tamper with syscalls. If you tamper wrong, then you lose.
Kees, what do you think about reversing the whole thing so that
ptracers always see the original syscall?
What do you mean by "reversing"? The interactions I see here are:
PTRACE_SYSCALL
SECCOMP_RET_ERRNO
SECCOMP_RET_TRACE
SECCOMP_RET_TRAP
Both ptrace and seccomp will trigger via _TIF_WORK_SYSCALL_ENTRY. Only
ptrace will trigger via _TIF_WORK_SYSCALL_EXIT.
For SECCOMP_RET_ERRNO to work, we must skip the syscall, as mentioned earlier:
arch/x86/kernel/entry_32.S ...
syscall_trace_entry:
movl $-ENOSYS,PT_EAX(%esp)
movl %esp, %eax
call syscall_trace_enter
/* What it returned is what we'll actually use. */
cmpl $(NR_syscalls), %eax
jnae syscall_call
jmp syscall_exit
END(syscall_trace_entry)
Both before and after the 2-phase change, syscall_trace_enter would
return -1 if it hit SECCOMP_RET_ERRNO, before calling
tracehook_report_syscall_entry. On exit, if PTRACE_SYSCALL, we'd hit
tracehook_report_syscall_exit during syscall_trace_leave, which means
a ptracer will see a syscall-exit-stop without a matching
syscall-enter-stop.
Using SECCOMP_RET_TRACE with PTRACE_SYSCALL in place seems totally
crazy, as the ptracer would need to be the same program, and if it
chose to skip a syscall, it would be in the same place: it would see
PTRACE_EVENT_SECCOMP, then no syscall-enter-stop, then a
syscall-exit-stop. I think we can ignore this pathological case.
Using SECCOMP_RET_TRAP with PTRACE_SYSCALL also results in a skip,
which produces the same "only syscall-exit-stop seen" problem.
In the SECCOMP_RET_ERRNO case, the syscall nr doesn't change (and
isn't executed). In the SECCOMP_RET_TRAP case, the syscall nr doesn't
change (and isn't executed). In the SECCOMP_RET_TRACE, the syscall nr
_could_ change, but the ptracer would be doing it, so the crazy
situation around PTRACE_SYSCALL is probably safe to ignore (as long as
we document what is expected to happen).
So, the question is: should PTRACE_SYSCALL see a syscall that is _not_
being executed (due to seccomp)? Audit doesn't see it currently, and
neither does ptrace. I would argue that it should continue to not see
the syscall. That said, if it shouldn't be shown, we also shouldn't
trigger syscall-exit-stop. If you can convince me it should see
syscall-enter-stop, then I have two questions:
I think PTRACE_SYSCALL should see syscalls that are skipped due to
seccomp. I think that the exit event should see the modified errno,
if any, so that strace will show whatever the traced process thinks is
happening.
1) Do we accept that a ptracer can interfere with SECCOMP_RET_ERRNO? I
think we probably must, since it can already interfere via
syscall-exit-stop and change the errno.
I think this is fine.
And especially since a ptracer
can change syscalls during syscall-enter-stop to any syscall it wants,
bypassing seccomp. This condition is already documented.
If a ptracer (using PTRACE_SYSCALL) were to get the entry callback
before seccomp, then this oddity would go away, which might be a good
thing. A ptracer could change the syscall, but seccomp would based on
what the ptracer changed the syscall to.
2) What do we do with audit? Suddenly we have ptrace seeing a syscall
that audit doesn't?
Is this a problem? I'd be amazed if program uses both ptrace and
audit -- after all, audit is a global thing, and it only has one
implementation (AFAIK): auditd. auditd doesn't ptrace the world.
And an unrelated thought:
3) Can't we find some way to fix the inability of a ptracer to
distinguish between syscall-enter-stop and syscall-exit-stop?
Couldn't we add PTRACE_O_TRACESYSENTRY and PTRACE_O_TRACESYSEXIT along
the lines of PTRACE_O_TRACESYSGOOD?
--Andy
On Fri, Feb 6, 2015 at 11:32 AM, Andy Lutomirski [off-list ref] wrote:
On Fri, Feb 6, 2015 at 11:23 AM, Kees Cook [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 6:38 PM, Andy Lutomirski [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 6:32 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Thu, Feb 05, 2015 at 04:09:06PM -0800, Andy Lutomirski wrote:
quoted
On Thu, Feb 5, 2015 at 3:49 PM, Kees Cook [off-list ref] wrote:
quoted
On Thu, Feb 5, 2015 at 3:39 PM, Dmitry V. Levin [off-list ref] wrote:
[...]
quoted
quoted
quoted
There is a clear difference: before these changes, SECCOMP_RET_ERRNO used
to keep the syscall number unchanged and suppress syscall-exit-stop event,
which was awful because userspace cannot distinguish syscall-enter-stop
from syscall-exit-stop and therefore relies on the kernel that
syscall-enter-stop is followed by syscall-exit-stop (or tracee's death, etc.).
After these changes, SECCOMP_RET_ERRNO no longer causes syscall-exit-stop
events to be suppressed, but now the syscall number is lost.
Ah-ha! Okay, thanks, I understand now. I think this means seccomp
phase1 should not treat RET_ERRNO as a "skip" event. Andy, what do you
think here?
I still don't quite see how this change caused this.
I can play with
it a bit more. But RET_ERRNO *has* to be some kind of skip event,
because it needs to skip the syscall.
We could change this by treating RET_ERRNO as an instruction to enter
phase 2 and then asking for a skip in phase 2 without changing
orig_ax, but IMO this is pretty ugly.
I think this all kind of sucks. We're trying to run ptrace after
seccomp, so ptrace is seeing the syscalls as transformed by seccomp.
That means that if we use RET_TRAP, then ptrace will see the
possibly-modified syscall, if we use RET_ERRNO, then ptrace is (IMO
correctly given the current design) showing syscall -1, and if we use
RET_KILL, then ptrace just sees the process mysteriously die.
To save others the link reading: "Linus said he will make sure the no
syscall returns a value in -1 .. -4095 as a valid result so we can
savely test with -4095."
Strictly speaking (ISO C, "man 3 errno"), errno is supposed to be a
full int, though digging around I find this in include/linux/err.h:
/*
* Kernel pointers have redundant information, so we can use a
* scheme where we can return either an error code or a normal
* pointer with the same return value.
*
* This should be a per-architecture thing, to allow different
* error and pointer decisions.
*/
#define MAX_ERRNO 4095
#ifndef __ASSEMBLY__
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
But no architecture overrides this.
quoted
quoted
If it isn't too late, I'd recommend changing SECCOMP_RET_DATA mask
applied in SECCOMP_RET_ERRNO case from current 0xffff to 0xfff.
I'm not opposed to this. I would want to more explicitly document the
4095 max value in man pages, though.
quoted
I think this is solidly in the "don't do that" category. Seccomp lets
you tamper with syscalls. If you tamper wrong, then you lose.
Kees, what do you think about reversing the whole thing so that
ptracers always see the original syscall?
What do you mean by "reversing"? The interactions I see here are:
PTRACE_SYSCALL
SECCOMP_RET_ERRNO
SECCOMP_RET_TRACE
SECCOMP_RET_TRAP
Both ptrace and seccomp will trigger via _TIF_WORK_SYSCALL_ENTRY. Only
ptrace will trigger via _TIF_WORK_SYSCALL_EXIT.
For SECCOMP_RET_ERRNO to work, we must skip the syscall, as mentioned earlier:
arch/x86/kernel/entry_32.S ...
syscall_trace_entry:
movl $-ENOSYS,PT_EAX(%esp)
movl %esp, %eax
call syscall_trace_enter
/* What it returned is what we'll actually use. */
cmpl $(NR_syscalls), %eax
jnae syscall_call
jmp syscall_exit
END(syscall_trace_entry)
Both before and after the 2-phase change, syscall_trace_enter would
return -1 if it hit SECCOMP_RET_ERRNO, before calling
tracehook_report_syscall_entry. On exit, if PTRACE_SYSCALL, we'd hit
tracehook_report_syscall_exit during syscall_trace_leave, which means
a ptracer will see a syscall-exit-stop without a matching
syscall-enter-stop.
Using SECCOMP_RET_TRACE with PTRACE_SYSCALL in place seems totally
crazy, as the ptracer would need to be the same program, and if it
chose to skip a syscall, it would be in the same place: it would see
PTRACE_EVENT_SECCOMP, then no syscall-enter-stop, then a
syscall-exit-stop. I think we can ignore this pathological case.
Using SECCOMP_RET_TRAP with PTRACE_SYSCALL also results in a skip,
which produces the same "only syscall-exit-stop seen" problem.
In the SECCOMP_RET_ERRNO case, the syscall nr doesn't change (and
isn't executed). In the SECCOMP_RET_TRAP case, the syscall nr doesn't
change (and isn't executed). In the SECCOMP_RET_TRACE, the syscall nr
_could_ change, but the ptracer would be doing it, so the crazy
situation around PTRACE_SYSCALL is probably safe to ignore (as long as
we document what is expected to happen).
So, the question is: should PTRACE_SYSCALL see a syscall that is _not_
being executed (due to seccomp)? Audit doesn't see it currently, and
neither does ptrace. I would argue that it should continue to not see
the syscall. That said, if it shouldn't be shown, we also shouldn't
trigger syscall-exit-stop. If you can convince me it should see
syscall-enter-stop, then I have two questions:
I think PTRACE_SYSCALL should see syscalls that are skipped due to
seccomp. I think that the exit event should see the modified errno,
if any, so that strace will show whatever the traced process thinks is
happening.
quoted
1) Do we accept that a ptracer can interfere with SECCOMP_RET_ERRNO? I
think we probably must, since it can already interfere via
syscall-exit-stop and change the errno.
I think this is fine.
quoted
And especially since a ptracer
can change syscalls during syscall-enter-stop to any syscall it wants,
bypassing seccomp. This condition is already documented.
If a ptracer (using PTRACE_SYSCALL) were to get the entry callback
before seccomp, then this oddity would go away, which might be a good
thing. A ptracer could change the syscall, but seccomp would based on
what the ptracer changed the syscall to.
I want kill events to trigger immediately. I don't want to leave the
ptrace surface available on a SECCOMP_RET_KILL. So maybe it can be
seccomp phase 1, then ptrace, then seccomp phase 2? And pass more
information between phases to determine how things should behave
beyond just "skip"?
quoted
2) What do we do with audit? Suddenly we have ptrace seeing a syscall
that audit doesn't?
Is this a problem? I'd be amazed if program uses both ptrace and
audit -- after all, audit is a global thing, and it only has one
implementation (AFAIK): auditd. auditd doesn't ptrace the world.
quoted
And an unrelated thought:
3) Can't we find some way to fix the inability of a ptracer to
distinguish between syscall-enter-stop and syscall-exit-stop?
Couldn't we add PTRACE_O_TRACESYSENTRY and PTRACE_O_TRACESYSEXIT along
the lines of PTRACE_O_TRACESYSGOOD?
That might be a nice idea. I haven't written a test to see, but what
does PTRACE_GETEVENTMSG return on syscall-enter/exit-stop? If we can't
add something there, then yeah, adding PTRACE_O_TRACESYSENTRY and
PTRACE_O_TRACESYSEXIT with their own event msgs would be nice. Could
even add the syscall nr to the event msg so ptracers don't have to dig
around in per-arch registers, too.
-Kees
--
Kees Cook
Chrome OS Security
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-02-06 20:13:06
On Fri, Feb 6, 2015 at 12:07 PM, Kees Cook [off-list ref] wrote:
On Fri, Feb 6, 2015 at 11:32 AM, Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 11:23 AM, Kees Cook [off-list ref] wrote:
quoted
And especially since a ptracer
can change syscalls during syscall-enter-stop to any syscall it wants,
bypassing seccomp. This condition is already documented.
If a ptracer (using PTRACE_SYSCALL) were to get the entry callback
before seccomp, then this oddity would go away, which might be a good
thing. A ptracer could change the syscall, but seccomp would based on
what the ptracer changed the syscall to.
I want kill events to trigger immediately. I don't want to leave the
ptrace surface available on a SECCOMP_RET_KILL. So maybe it can be
seccomp phase 1, then ptrace, then seccomp phase 2? And pass more
information between phases to determine how things should behave
beyond just "skip"?
I thought so too, originally, but I'm far less convinced now, for two reasons:
1. I think that a lot of filters these days use RET_ERRNO heavily, so
this won't benefit them.
2. I'm not convinced it really reduces the attack surface for anyone.
Unless your filter is literally "return SECCOMP_RET_KILL", then the
seccomp-filtered task can always cause the ptracer to get a pair of
syscall notifications. Also, the task can send itself signals (using
page faults, breakpoints, etc) and cause ptrace events via other
paths.
--Andy
On Fri, Feb 6, 2015 at 12:12 PM, Andy Lutomirski [off-list ref] wrote:
On Fri, Feb 6, 2015 at 12:07 PM, Kees Cook [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 11:32 AM, Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 11:23 AM, Kees Cook [off-list ref] wrote:
quoted
And especially since a ptracer
can change syscalls during syscall-enter-stop to any syscall it wants,
bypassing seccomp. This condition is already documented.
If a ptracer (using PTRACE_SYSCALL) were to get the entry callback
before seccomp, then this oddity would go away, which might be a good
thing. A ptracer could change the syscall, but seccomp would based on
what the ptracer changed the syscall to.
I want kill events to trigger immediately. I don't want to leave the
ptrace surface available on a SECCOMP_RET_KILL. So maybe it can be
seccomp phase 1, then ptrace, then seccomp phase 2? And pass more
information between phases to determine how things should behave
beyond just "skip"?
I thought so too, originally, but I'm far less convinced now, for two reasons:
1. I think that a lot of filters these days use RET_ERRNO heavily, so
this won't benefit them.
2. I'm not convinced it really reduces the attack surface for anyone.
Unless your filter is literally "return SECCOMP_RET_KILL", then the
seccomp-filtered task can always cause the ptracer to get a pair of
syscall notifications. Also, the task can send itself signals (using
page faults, breakpoints, etc) and cause ptrace events via other
paths.
What are you thinking for a solution?
As for capping SECCOMP_RET_ERRNO to MAX_ERRNO, how about this (sorry
if gmail butchers the paste):
@@ -629,7 +629,9 @@ static u32 __seccomp_phase1_filter(int this_syscall, structswitch(action){caseSECCOMP_RET_ERRNO:-/* Set the low-order 16-bits as a errno. */+/* Set the low-order bits as a errno. */+if(data>MAX_ERRNO)+data=MAX_ERRNO;syscall_set_return_value(current,task_pt_regs(current),-data,0);gotoskip;
From: Andy Lutomirski <luto@amacapital.net> Date: 2015-02-06 20:20:44
On Fri, Feb 6, 2015 at 12:16 PM, Kees Cook [off-list ref] wrote:
On Fri, Feb 6, 2015 at 12:12 PM, Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 12:07 PM, Kees Cook [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 11:32 AM, Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 11:23 AM, Kees Cook [off-list ref] wrote:
quoted
And especially since a ptracer
can change syscalls during syscall-enter-stop to any syscall it wants,
bypassing seccomp. This condition is already documented.
If a ptracer (using PTRACE_SYSCALL) were to get the entry callback
before seccomp, then this oddity would go away, which might be a good
thing. A ptracer could change the syscall, but seccomp would based on
what the ptracer changed the syscall to.
I want kill events to trigger immediately. I don't want to leave the
ptrace surface available on a SECCOMP_RET_KILL. So maybe it can be
seccomp phase 1, then ptrace, then seccomp phase 2? And pass more
information between phases to determine how things should behave
beyond just "skip"?
I thought so too, originally, but I'm far less convinced now, for two reasons:
1. I think that a lot of filters these days use RET_ERRNO heavily, so
this won't benefit them.
2. I'm not convinced it really reduces the attack surface for anyone.
Unless your filter is literally "return SECCOMP_RET_KILL", then the
seccomp-filtered task can always cause the ptracer to get a pair of
syscall notifications. Also, the task can send itself signals (using
page faults, breakpoints, etc) and cause ptrace events via other
paths.
What are you thinking for a solution?
I'm writing a patch now. It's an ABI break, but this thread seems to
show that the ABI was somewhat useless before the split-phase changes,
and it's differently broken now, so I would be surprised if the change
broke anything that was currently working. I'll send it later today,
hopefully.
quoted hunk
As for capping SECCOMP_RET_ERRNO to MAX_ERRNO, how about this (sorry
if gmail butchers the paste):
@@ -629,7 +629,9 @@ static u32 __seccomp_phase1_filter(int this_syscall, structswitch(action){caseSECCOMP_RET_ERRNO:-/* Set the low-order 16-bits as a errno. */+/* Set the low-order bits as a errno. */+if(data>MAX_ERRNO)+data=MAX_ERRNO;syscall_set_return_value(current,task_pt_regs(current),-data,0);gotoskip;
I'm fine with this, but I'm not entirely convinced it solves a
problem. SECCOMP_RET_ERRNO | 5000 didn't work before, and it doesn't
work now. Admittedly, the new failure mode is possibly better.
--Andy
From: Dmitry V. Levin <hidden> Date: 2015-02-06 23:17:24
On Fri, Feb 06, 2015 at 12:07:03PM -0800, Kees Cook wrote:
On Fri, Feb 6, 2015 at 11:32 AM, Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 11:23 AM, Kees Cook [off-list ref] wrote:
[...]
quoted
quoted
And an unrelated thought:
3) Can't we find some way to fix the inability of a ptracer to
distinguish between syscall-enter-stop and syscall-exit-stop?
Couldn't we add PTRACE_O_TRACESYSENTRY and PTRACE_O_TRACESYSEXIT along
the lines of PTRACE_O_TRACESYSGOOD?
That might be a nice idea. I haven't written a test to see, but what
does PTRACE_GETEVENTMSG return on syscall-enter/exit-stop?
The value returned by PTRACE_GETEVENTMSG is the value set along with the
latest PTRACE_EVENT_*.
In case of syscall-enter/exit-stop (which is not a PTRACE_EVENT_*),
there is no particular value set for PTRACE_GETEVENTMSG.
--
ldv
On Fri, Feb 6, 2015 at 3:17 PM, Dmitry V. Levin [off-list ref] wrote:
On Fri, Feb 06, 2015 at 12:07:03PM -0800, Kees Cook wrote:
quoted
On Fri, Feb 6, 2015 at 11:32 AM, Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 11:23 AM, Kees Cook [off-list ref] wrote:
[...]
quoted
quoted
quoted
And an unrelated thought:
3) Can't we find some way to fix the inability of a ptracer to
distinguish between syscall-enter-stop and syscall-exit-stop?
Couldn't we add PTRACE_O_TRACESYSENTRY and PTRACE_O_TRACESYSEXIT along
the lines of PTRACE_O_TRACESYSGOOD?
That might be a nice idea. I haven't written a test to see, but what
does PTRACE_GETEVENTMSG return on syscall-enter/exit-stop?
The value returned by PTRACE_GETEVENTMSG is the value set along with the
latest PTRACE_EVENT_*.
In case of syscall-enter/exit-stop (which is not a PTRACE_EVENT_*),
there is no particular value set for PTRACE_GETEVENTMSG.
Could we define one to help distinguish?
-Kees
--
Kees Cook
Chrome OS Security
From: Dmitry V. Levin <hidden> Date: 2015-02-07 03:04:56
On Fri, Feb 06, 2015 at 05:07:41PM -0800, Kees Cook wrote:
On Fri, Feb 6, 2015 at 3:17 PM, Dmitry V. Levin [off-list ref] wrote:
quoted
On Fri, Feb 06, 2015 at 12:07:03PM -0800, Kees Cook wrote:
quoted
On Fri, Feb 6, 2015 at 11:32 AM, Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Feb 6, 2015 at 11:23 AM, Kees Cook [off-list ref] wrote:
[...]
quoted
quoted
quoted
And an unrelated thought:
3) Can't we find some way to fix the inability of a ptracer to
distinguish between syscall-enter-stop and syscall-exit-stop?
Couldn't we add PTRACE_O_TRACESYSENTRY and PTRACE_O_TRACESYSEXIT along
the lines of PTRACE_O_TRACESYSGOOD?
That might be a nice idea. I haven't written a test to see, but what
does PTRACE_GETEVENTMSG return on syscall-enter/exit-stop?
The value returned by PTRACE_GETEVENTMSG is the value set along with the
latest PTRACE_EVENT_*.
In case of syscall-enter/exit-stop (which is not a PTRACE_EVENT_*),
there is no particular value set for PTRACE_GETEVENTMSG.
Could we define one to help distinguish?
I suppose we could define one, but performing extra PTRACE_GETEVENTMSG
for every syscall-stop may be too expensive.
For example, strace makes about 4.5 syscalls per syscall-stop.
The minimum is 4 syscalls: wait4, PTRACE_GETREGSET, write, and PTRACE_SYSCALL;
processing some syscall-stops may require additional process_vm_readv calls.
That is, forcing strace to make extra PTRACE_GETEVENTMSG per syscall-stop
would result to about 20% more syscalls per syscall-stop, that is a
noticeable cost.
A better alternative is to define an event that wouldn't require this
extra PTRACE_GETEVENTMSG per syscall-stop. For example, it could be a
PTRACE_EVENT_SYSCALL_ENTRY and/or PTRACE_EVENT_SYSCALL_EXIT. In practice,
adding just one of these two events would be enough to distinguish two
kinds of syscall-stops. Adding two events would look less surprising,
though.
If the decision would be to add both events, I'd recommend adding just one
new option to cover both events - there is a room only for 32 different
PTRACE_O_* options.
--
ldv
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2015-02-06 20:12:48
On 02/06/2015 11:23 AM, Kees Cook wrote:
Strictly speaking (ISO C, "man 3 errno"), errno is supposed to be a
full int, though digging around I find this in include/linux/err.h:
That doesn't mean the kernel has to support them.
/*
* Kernel pointers have redundant information, so we can use a
* scheme where we can return either an error code or a normal
* pointer with the same return value.
*
* This should be a per-architecture thing, to allow different
* error and pointer decisions.
*/
#define MAX_ERRNO 4095
#ifndef __ASSEMBLY__
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
But no architecture overrides this.
We used to have a much lower value, that was per-architecture, in order
to optimize the resulting assembly (e.g. 8-bit immediates on x86). This
didn't work as the number of errnos increased. The other motivation was
probably binary compatibility with other Unices, which was an idea for a
while.
-hpa
From: Andy Lutomirski <luto@amacapital.net> Date: 2014-09-05 22:14:34
For slowpath syscalls, we initialize regs->ax to -ENOSYS and stick
the syscall number into regs->orig_ax prior to any possible tracing
and syscall execution. This is user-visible ABI used by ptrace
syscall emulation and seccomp.
For fastpath syscalls, there's no good reason not to do the same
thing. It's even slightly simpler than what we're currently doing.
It probably has no measureable performance impact. It should have
no user-visible effect.
The purpose of this patch is to prepare for two-phase syscall
tracing, in which the first phase might modify the saved RAX without
leaving the fast path. This change is just subtle enough that I'm
keeping it separate.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/include/asm/calling.h | 6 +++++-
arch/x86/kernel/entry_64.S | 13 ++++---------
2 files changed, 9 insertions(+), 10 deletions(-)
@@ -85,7 +85,7 @@ For 32-bit we have the following conventions - kernel is built with#define ARGOFFSET R11#define SWFRAME ORIG_RAX-.macroSAVE_ARGSaddskip=0,save_rcx=1,save_r891011=1+.macroSAVE_ARGSaddskip=0,save_rcx=1,save_r891011=1,rax_enosys=0subq$9*8+\addskip,%rspCFI_ADJUST_CFA_OFFSET9*8+\addskipmovq_cfirdi,8*8
@@ -96,7 +96,11 @@ For 32-bit we have the following conventions - kernel is built withmovq_cfircx,5*8.endif+.if\rax_enosys+movq$-ENOSYS,4*8(%rsp)+.elsemovq_cfirax,4*8+.endif.if\save_r891011movq_cfir8,3*8
From: Andy Lutomirski <luto@amacapital.net> Date: 2014-09-05 22:14:38
On KVM on my box, this reduces the overhead from an always-accept
seccomp filter from ~130ns to ~17ns. Most of that comes from
avoiding IRET on every syscall when seccomp is enabled.
In extremely approximate hacked-up benchmarking, just bypassing IRET
saves about 80ns, so there's another 43ns of savings here from
simplifying the seccomp path.
The diffstat is also rather nice :)
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/kernel/entry_64.S | 38 +++++++++++++++-----------------------
1 file changed, 15 insertions(+), 23 deletions(-)
On Fri, Sep 5, 2014 at 3:13 PM, Andy Lutomirski [off-list ref] wrote:
This applies to:
git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git seccomp-fastpath
Gitweb:
https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=seccomp/fastpath
This is both a cleanup and a speedup. It reduces overhead due to
installing a trivial seccomp filter by 87%. The speedup comes from
avoiding the full syscall tracing mechanism for filters that don't
return SECCOMP_RET_TRACE.
This series depends on splitting the seccomp hooks into two phases.
The first phase evaluates the filter; it can skip syscalls, allow
them, kill the calling task, or pass a u32 to the second phase. The
second phase requires a full tracing context, and it sends ptrace
events if necessary. The seccomp core part is in Kees' seccomp/fastpath
tree.
These patches implement a similar split for the x86 syscall
entry work. The C callback is invoked in two phases: the first has
only a partial frame, and it can request phase 2 processing with a
full frame.
Finally, I switch the 64-bit system_call code to use the new split
entry work. This is a net deletion of assembly code: it replaces
all of the audit entry muck.
In the process, I fixed some bugs.
If this is acceptable, someone can do the same tweak for the
ia32entry and entry_32 code.
This passes all seccomp tests that I know of.
Changes from v4:
- Rebased (which seems to have been a no-op)
- Fixed embarrassing bug that broke allnoconfig
(patch 3 was missing an ifdef).
Changes from v3:
- Dropped the core seccomp changes from the email -- Kees has applied them.
- Add patch 2 (the TIF_NOHZ change).
- Fix TIF_NOHZ in the two-phase entry code (thanks, Oleg).
Changes from v2:
- Fixed 32-bit x86 build (and the tests pass).
- Put the doc patch where it belongs.
Changes from v1:
- Rebased on top of Kees' shiny new seccomp tree (no effect on the x86
part).
- Improved patch 6 vs patch 7 split (thanks Alexei!)
- Fixed bogus -ENOSYS in patch 5 (thanks Kees!)
- Improved changelog message in patch 6.
Changes from RFC version:
- The first three patches are more or less the same
- The rest is more or less a rewrite
Andy Lutomirski (5):
x86,x32,audit: Fix x32's AUDIT_ARCH wrt audit
x86,entry: Only call user_exit if TIF_NOHZ
x86: Split syscall_trace_enter into two phases
x86_64,entry: Treat regs->ax the same in fastpath and slowpath
syscalls
x86_64,entry: Use split-phase syscall_trace_enter for 64-bit syscalls
arch/x86/include/asm/calling.h | 6 +-
arch/x86/include/asm/ptrace.h | 5 ++
arch/x86/kernel/entry_64.S | 51 +++++--------
arch/x86/kernel/ptrace.c | 165 +++++++++++++++++++++++++++++++++--------
4 files changed, 164 insertions(+), 63 deletions(-)
Consider the series:
Acked-by: Kees Cook <redacted>
As far as doing pulls, Peter, can you take the seccomp change from my
tree as well? It makes sense to land all of this together.
Thanks!
-Kees
--
Kees Cook
Chrome OS Security