| Message ID | 1329845435-2313-7-git-send-email-wad@chromium.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, February 21, 2012 18:30, Will Drewry wrote: > This change enables SIGSYS, defines _sigfields._sigsys, and adds > x86 (compat) arch support. _sigsys defines fields which allow > a signal handler to receive the triggering system call number, > the relevant AUDIT_ARCH_* value for that number, and the address > of the callsite. > > To ensure that SIGSYS delivery occurs on return from the triggering > system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. I'm > this is enough to ensure it will be synchronous or if it is explicitly > required to ensure an immediate delivery of the signal upon return from > the blocked system call. > > The first consumer of SIGSYS would be seccomp filter. In particular, > a filter program could specify a new return value, SECCOMP_RET_TRAP, > which would result in the system call being denied and the calling > thread signaled. This also means that implementing arch-specific > support can be dependent upon HAVE_ARCH_SECCOMP_FILTER. I think others said this is useful, but I don't see how. Easier debugging compared to checking return values? I suppose SIGSYS can be blocked, so there is no guarantee the process will be killed. > v10: - first version based on suggestion > > Suggested-by: H. Peter Anvin <hpa@zytor.com> > Signed-off-by: Will Drewry <wad@chromium.org> > --- > arch/x86/ia32/ia32_signal.c | 4 ++++ > arch/x86/include/asm/ia32.h | 6 ++++++ > include/asm-generic/siginfo.h | 18 ++++++++++++++++++ > kernel/signal.c | 2 +- > 4 files changed, 29 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c > index 6557769..c81d2c7 100644 > --- a/arch/x86/ia32/ia32_signal.c > +++ b/arch/x86/ia32/ia32_signal.c > @@ -73,6 +73,10 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t > *from) > switch (from->si_code >> 16) { > case __SI_FAULT >> 16: > break; > + case __SI_SYS >> 16: > + put_user_ex(from->si_syscall, &to->si_syscall); > + put_user_ex(from->si_arch, &to->si_arch); > + break; > case __SI_CHLD >> 16: > put_user_ex(from->si_utime, &to->si_utime); > put_user_ex(from->si_stime, &to->si_stime); > diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h > index 1f7e625..541485f 100644 > --- a/arch/x86/include/asm/ia32.h > +++ b/arch/x86/include/asm/ia32.h > @@ -126,6 +126,12 @@ typedef struct compat_siginfo { > int _band; /* POLL_IN, POLL_OUT, POLL_MSG */ > int _fd; > } _sigpoll; > + > + struct { > + unsigned int _call_addr; /* calling insn */ Why an int here, but a pointer below? > + int _syscall; /* triggering system call number */ > + unsigned int _arch; /* AUDIT_ARCH_* of syscall */ > + } _sigsys; > } _sifields; > } compat_siginfo_t; > > diff --git a/include/asm-generic/siginfo.h b/include/asm-generic/siginfo.h > index 0dd4e87..a83b478 100644 > --- a/include/asm-generic/siginfo.h > +++ b/include/asm-generic/siginfo.h > @@ -90,6 +90,13 @@ typedef struct siginfo { > __ARCH_SI_BAND_T _band; /* POLL_IN, POLL_OUT, POLL_MSG */ > int _fd; > } _sigpoll; > + > + /* SIGSYS */ > + struct { > + void __user *_call_addr; /* calling insn */ Is this a user instruction pointer or a filter instruction? > + int _syscall; /* triggering system call number */ > + unsigned int _arch; /* AUDIT_ARCH_* of syscall */ > + } _sigsys; > } _sifields; > } siginfo_t; > > @@ -116,6 +123,9 @@ typedef struct siginfo { > #define si_addr_lsb _sifields._sigfault._addr_lsb > #define si_band _sifields._sigpoll._band > #define si_fd _sifields._sigpoll._fd > +#define si_call_addr _sifields._sigsys._call_addr > +#define si_syscall _sifields._sigsys._syscall > +#define si_arch _sifields._sigsys._arch > > #ifdef __KERNEL__ > #define __SI_MASK 0xffff0000u > @@ -126,6 +136,7 @@ typedef struct siginfo { > #define __SI_CHLD (4 << 16) > #define __SI_RT (5 << 16) > #define __SI_MESGQ (6 << 16) > +#define __SI_SYS (7 << 16) > #define __SI_CODE(T,N) ((T) | ((N) & 0xffff)) > #else > #define __SI_KILL 0 > @@ -135,6 +146,7 @@ typedef struct siginfo { > #define __SI_CHLD 0 > #define __SI_RT 0 > #define __SI_MESGQ 0 > +#define __SI_SYS 0 > #define __SI_CODE(T,N) (N) > #endif > > @@ -232,6 +244,12 @@ typedef struct siginfo { > #define NSIGPOLL 6 > > /* > + * SIGSYS si_codes > + */ > +#define SYS_SECCOMP (__SI_SYS|1) /* seccomp triggered */ > +#define NSIGSYS 1 > + > +/* > * sigevent definitions > * > * It seems likely that SIGEV_THREAD will have to be handled from > diff --git a/kernel/signal.c b/kernel/signal.c > index c73c428..7573819 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -160,7 +160,7 @@ void recalc_sigpending(void) > > #define SYNCHRONOUS_MASK \ > (sigmask(SIGSEGV) | sigmask(SIGBUS) | sigmask(SIGILL) | \ > - sigmask(SIGTRAP) | sigmask(SIGFPE)) > + sigmask(SIGTRAP) | sigmask(SIGFPE) | sigmask(SIGSYS)) > > int next_signal(struct sigpending *pending, sigset_t *mask) > { > -- Greetings, Indan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 22, 2012 at 2:34 AM, Indan Zupancic <indan@nul.nu> wrote: > On Tue, February 21, 2012 18:30, Will Drewry wrote: >> This change enables SIGSYS, defines _sigfields._sigsys, and adds >> x86 (compat) arch support. _sigsys defines fields which allow >> a signal handler to receive the triggering system call number, >> the relevant AUDIT_ARCH_* value for that number, and the address >> of the callsite. >> >> To ensure that SIGSYS delivery occurs on return from the triggering >> system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. I'm >> this is enough to ensure it will be synchronous or if it is explicitly >> required to ensure an immediate delivery of the signal upon return from >> the blocked system call. >> >> The first consumer of SIGSYS would be seccomp filter. In particular, >> a filter program could specify a new return value, SECCOMP_RET_TRAP, >> which would result in the system call being denied and the calling >> thread signaled. This also means that implementing arch-specific >> support can be dependent upon HAVE_ARCH_SECCOMP_FILTER. > > I think others said this is useful, but I don't see how. Easier > debugging compared to checking return values? > > I suppose SIGSYS can be blocked, so there is no guarantee the process > will be killed. Yeah, this allows for in-process system call emulation, if desired, or for the process to dump core/etc. With RET_ERRNO or RET_KILL, there isn't any feedback to the system about the state of the process. Kill populates audit_seccomp and dmesg, but if the application user/developer isn't the system admin, installing audit bits or checking system logs seems onerous. >> v10: - first version based on suggestion >> >> Suggested-by: H. Peter Anvin <hpa@zytor.com> >> Signed-off-by: Will Drewry <wad@chromium.org> >> --- >> arch/x86/ia32/ia32_signal.c | 4 ++++ >> arch/x86/include/asm/ia32.h | 6 ++++++ >> include/asm-generic/siginfo.h | 18 ++++++++++++++++++ >> kernel/signal.c | 2 +- >> 4 files changed, 29 insertions(+), 1 deletions(-) >> >> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c >> index 6557769..c81d2c7 100644 >> --- a/arch/x86/ia32/ia32_signal.c >> +++ b/arch/x86/ia32/ia32_signal.c >> @@ -73,6 +73,10 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t >> *from) >> switch (from->si_code >> 16) { >> case __SI_FAULT >> 16: >> break; >> + case __SI_SYS >> 16: >> + put_user_ex(from->si_syscall, &to->si_syscall); >> + put_user_ex(from->si_arch, &to->si_arch); >> + break; >> case __SI_CHLD >> 16: >> put_user_ex(from->si_utime, &to->si_utime); >> put_user_ex(from->si_stime, &to->si_stime); >> diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h >> index 1f7e625..541485f 100644 >> --- a/arch/x86/include/asm/ia32.h >> +++ b/arch/x86/include/asm/ia32.h >> @@ -126,6 +126,12 @@ typedef struct compat_siginfo { >> int _band; /* POLL_IN, POLL_OUT, POLL_MSG */ >> int _fd; >> } _sigpoll; >> + >> + struct { >> + unsigned int _call_addr; /* calling insn */ > > Why an int here, but a pointer below? This is the compat version and it expects to just use an unsigned int (see the _addr entry in _sigfault earlier in the same file). >> + int _syscall; /* triggering system call number */ >> + unsigned int _arch; /* AUDIT_ARCH_* of syscall */ >> + } _sigsys; >> } _sifields; >> } compat_siginfo_t; >> >> diff --git a/include/asm-generic/siginfo.h b/include/asm-generic/siginfo.h >> index 0dd4e87..a83b478 100644 >> --- a/include/asm-generic/siginfo.h >> +++ b/include/asm-generic/siginfo.h >> @@ -90,6 +90,13 @@ typedef struct siginfo { >> __ARCH_SI_BAND_T _band; /* POLL_IN, POLL_OUT, POLL_MSG */ >> int _fd; >> } _sigpoll; >> + >> + /* SIGSYS */ >> + struct { >> + void __user *_call_addr; /* calling insn */ > > Is this a user instruction pointer or a filter instruction? User instruction pointer, I'll clarify. >> + int _syscall; /* triggering system call number */ >> + unsigned int _arch; /* AUDIT_ARCH_* of syscall */ >> + } _sigsys; >> } _sifields; >> } siginfo_t; >> >> @@ -116,6 +123,9 @@ typedef struct siginfo { >> #define si_addr_lsb _sifields._sigfault._addr_lsb >> #define si_band _sifields._sigpoll._band >> #define si_fd _sifields._sigpoll._fd >> +#define si_call_addr _sifields._sigsys._call_addr >> +#define si_syscall _sifields._sigsys._syscall >> +#define si_arch _sifields._sigsys._arch >> >> #ifdef __KERNEL__ >> #define __SI_MASK 0xffff0000u >> @@ -126,6 +136,7 @@ typedef struct siginfo { >> #define __SI_CHLD (4 << 16) >> #define __SI_RT (5 << 16) >> #define __SI_MESGQ (6 << 16) >> +#define __SI_SYS (7 << 16) >> #define __SI_CODE(T,N) ((T) | ((N) & 0xffff)) >> #else >> #define __SI_KILL 0 >> @@ -135,6 +146,7 @@ typedef struct siginfo { >> #define __SI_CHLD 0 >> #define __SI_RT 0 >> #define __SI_MESGQ 0 >> +#define __SI_SYS 0 >> #define __SI_CODE(T,N) (N) >> #endif >> >> @@ -232,6 +244,12 @@ typedef struct siginfo { >> #define NSIGPOLL 6 >> >> /* >> + * SIGSYS si_codes >> + */ >> +#define SYS_SECCOMP (__SI_SYS|1) /* seccomp triggered */ >> +#define NSIGSYS 1 >> + >> +/* >> * sigevent definitions >> * >> * It seems likely that SIGEV_THREAD will have to be handled from >> diff --git a/kernel/signal.c b/kernel/signal.c >> index c73c428..7573819 100644 >> --- a/kernel/signal.c >> +++ b/kernel/signal.c >> @@ -160,7 +160,7 @@ void recalc_sigpending(void) >> >> #define SYNCHRONOUS_MASK \ >> (sigmask(SIGSEGV) | sigmask(SIGBUS) | sigmask(SIGILL) | \ >> - sigmask(SIGTRAP) | sigmask(SIGFPE)) >> + sigmask(SIGTRAP) | sigmask(SIGFPE) | sigmask(SIGSYS)) >> >> int next_signal(struct sigpending *pending, sigset_t *mask) >> { >> -- thanks! will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 22, 2012 at 11:48 AM, Will Drewry <wad@chromium.org> wrote: > On Wed, Feb 22, 2012 at 2:34 AM, Indan Zupancic <indan@nul.nu> wrote: >> On Tue, February 21, 2012 18:30, Will Drewry wrote: >>> This change enables SIGSYS, defines _sigfields._sigsys, and adds >>> x86 (compat) arch support. _sigsys defines fields which allow >>> a signal handler to receive the triggering system call number, >>> the relevant AUDIT_ARCH_* value for that number, and the address >>> of the callsite. >>> >>> To ensure that SIGSYS delivery occurs on return from the triggering >>> system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. I'm >>> this is enough to ensure it will be synchronous or if it is explicitly >>> required to ensure an immediate delivery of the signal upon return from >>> the blocked system call. >>> >>> The first consumer of SIGSYS would be seccomp filter. In particular, >>> a filter program could specify a new return value, SECCOMP_RET_TRAP, >>> which would result in the system call being denied and the calling >>> thread signaled. This also means that implementing arch-specific >>> support can be dependent upon HAVE_ARCH_SECCOMP_FILTER. >> >> I think others said this is useful, but I don't see how. Easier >> debugging compared to checking return values? >> >> I suppose SIGSYS can be blocked, so there is no guarantee the process >> will be killed. > > Yeah, this allows for in-process system call emulation, if desired, or > for the process to dump core/etc. With RET_ERRNO or RET_KILL, there > isn't any feedback to the system about the state of the process. Kill > populates audit_seccomp and dmesg, but if the application > user/developer isn't the system admin, installing audit bits or > checking system logs seems onerous. [Warning: this suggestion may be bad for any number of reasons] I wonder if it would be helpful to change the semantics of RET_KILL slightly. Rather than killing via do_exit, what if it killed via a forcibly-fatal SIGSYS? That way, the parent's waitid() / SIGCHLD would indicate CLD_KILLED with si_status == SIGSYS. The parent could check that and report that the child was probably compromised. --Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 22, 2012 at 3:38 PM, Andrew Lutomirski <luto@mit.edu> wrote: > On Wed, Feb 22, 2012 at 11:48 AM, Will Drewry <wad@chromium.org> wrote: >> On Wed, Feb 22, 2012 at 2:34 AM, Indan Zupancic <indan@nul.nu> wrote: >>> On Tue, February 21, 2012 18:30, Will Drewry wrote: >>>> This change enables SIGSYS, defines _sigfields._sigsys, and adds >>>> x86 (compat) arch support. _sigsys defines fields which allow >>>> a signal handler to receive the triggering system call number, >>>> the relevant AUDIT_ARCH_* value for that number, and the address >>>> of the callsite. >>>> >>>> To ensure that SIGSYS delivery occurs on return from the triggering >>>> system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. I'm >>>> this is enough to ensure it will be synchronous or if it is explicitly >>>> required to ensure an immediate delivery of the signal upon return from >>>> the blocked system call. >>>> >>>> The first consumer of SIGSYS would be seccomp filter. In particular, >>>> a filter program could specify a new return value, SECCOMP_RET_TRAP, >>>> which would result in the system call being denied and the calling >>>> thread signaled. This also means that implementing arch-specific >>>> support can be dependent upon HAVE_ARCH_SECCOMP_FILTER. >>> >>> I think others said this is useful, but I don't see how. Easier >>> debugging compared to checking return values? >>> >>> I suppose SIGSYS can be blocked, so there is no guarantee the process >>> will be killed. >> >> Yeah, this allows for in-process system call emulation, if desired, or >> for the process to dump core/etc. With RET_ERRNO or RET_KILL, there >> isn't any feedback to the system about the state of the process. Kill >> populates audit_seccomp and dmesg, but if the application >> user/developer isn't the system admin, installing audit bits or >> checking system logs seems onerous. > > [Warning: this suggestion may be bad for any number of reasons] > > I wonder if it would be helpful to change the semantics of RET_KILL > slightly. Rather than killing via do_exit, what if it killed via a > forcibly-fatal SIGSYS? That way, the parent's waitid() / SIGCHLD > would indicate CLD_KILLED with si_status == SIGSYS. The parent could > check that and report that the child was probably compromised. > > --Andy I'd prefer sticking with do_exit. This provides much less chance of things going wrong. A parent seeing a child killed with SIGKILL is already pretty distinct, IMO. -Kees
On Wed, Feb 22, 2012 at 5:53 PM, Kees Cook <keescook@chromium.org> wrote: > On Wed, Feb 22, 2012 at 3:38 PM, Andrew Lutomirski <luto@mit.edu> wrote: >> On Wed, Feb 22, 2012 at 11:48 AM, Will Drewry <wad@chromium.org> wrote: >>> On Wed, Feb 22, 2012 at 2:34 AM, Indan Zupancic <indan@nul.nu> wrote: >>>> On Tue, February 21, 2012 18:30, Will Drewry wrote: >>>>> This change enables SIGSYS, defines _sigfields._sigsys, and adds >>>>> x86 (compat) arch support. _sigsys defines fields which allow >>>>> a signal handler to receive the triggering system call number, >>>>> the relevant AUDIT_ARCH_* value for that number, and the address >>>>> of the callsite. >>>>> >>>>> To ensure that SIGSYS delivery occurs on return from the triggering >>>>> system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. I'm >>>>> this is enough to ensure it will be synchronous or if it is explicitly >>>>> required to ensure an immediate delivery of the signal upon return from >>>>> the blocked system call. >>>>> >>>>> The first consumer of SIGSYS would be seccomp filter. In particular, >>>>> a filter program could specify a new return value, SECCOMP_RET_TRAP, >>>>> which would result in the system call being denied and the calling >>>>> thread signaled. This also means that implementing arch-specific >>>>> support can be dependent upon HAVE_ARCH_SECCOMP_FILTER. >>>> >>>> I think others said this is useful, but I don't see how. Easier >>>> debugging compared to checking return values? >>>> >>>> I suppose SIGSYS can be blocked, so there is no guarantee the process >>>> will be killed. >>> >>> Yeah, this allows for in-process system call emulation, if desired, or >>> for the process to dump core/etc. With RET_ERRNO or RET_KILL, there >>> isn't any feedback to the system about the state of the process. Kill >>> populates audit_seccomp and dmesg, but if the application >>> user/developer isn't the system admin, installing audit bits or >>> checking system logs seems onerous. >> >> [Warning: this suggestion may be bad for any number of reasons] >> >> I wonder if it would be helpful to change the semantics of RET_KILL >> slightly. Rather than killing via do_exit, what if it killed via a >> forcibly-fatal SIGSYS? That way, the parent's waitid() / SIGCHLD >> would indicate CLD_KILLED with si_status == SIGSYS. The parent could >> check that and report that the child was probably compromised. >> >> --Andy > > I'd prefer sticking with do_exit. This provides much less chance of > things going wrong. A parent seeing a child killed with SIGKILL is > already pretty distinct, IMO. Hrm, it might be possible to do_exit(SIGSYS) which would be both. It looks like tsk->exit_code would be SIGSYS then, but I'll look a little more closely to see what that'll actually do. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 22, 2012 at 4:05 PM, Will Drewry <wad@chromium.org> wrote: > On Wed, Feb 22, 2012 at 5:53 PM, Kees Cook <keescook@chromium.org> wrote: >> On Wed, Feb 22, 2012 at 3:38 PM, Andrew Lutomirski <luto@mit.edu> wrote: >>> On Wed, Feb 22, 2012 at 11:48 AM, Will Drewry <wad@chromium.org> wrote: >>>> On Wed, Feb 22, 2012 at 2:34 AM, Indan Zupancic <indan@nul.nu> wrote: >>>>> On Tue, February 21, 2012 18:30, Will Drewry wrote: >>>>>> This change enables SIGSYS, defines _sigfields._sigsys, and adds >>>>>> x86 (compat) arch support. _sigsys defines fields which allow >>>>>> a signal handler to receive the triggering system call number, >>>>>> the relevant AUDIT_ARCH_* value for that number, and the address >>>>>> of the callsite. >>>>>> >>>>>> To ensure that SIGSYS delivery occurs on return from the triggering >>>>>> system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. I'm >>>>>> this is enough to ensure it will be synchronous or if it is explicitly >>>>>> required to ensure an immediate delivery of the signal upon return from >>>>>> the blocked system call. >>>>>> >>>>>> The first consumer of SIGSYS would be seccomp filter. In particular, >>>>>> a filter program could specify a new return value, SECCOMP_RET_TRAP, >>>>>> which would result in the system call being denied and the calling >>>>>> thread signaled. This also means that implementing arch-specific >>>>>> support can be dependent upon HAVE_ARCH_SECCOMP_FILTER. >>>>> >>>>> I think others said this is useful, but I don't see how. Easier >>>>> debugging compared to checking return values? >>>>> >>>>> I suppose SIGSYS can be blocked, so there is no guarantee the process >>>>> will be killed. >>>> >>>> Yeah, this allows for in-process system call emulation, if desired, or >>>> for the process to dump core/etc. With RET_ERRNO or RET_KILL, there >>>> isn't any feedback to the system about the state of the process. Kill >>>> populates audit_seccomp and dmesg, but if the application >>>> user/developer isn't the system admin, installing audit bits or >>>> checking system logs seems onerous. >>> >>> [Warning: this suggestion may be bad for any number of reasons] >>> >>> I wonder if it would be helpful to change the semantics of RET_KILL >>> slightly. Rather than killing via do_exit, what if it killed via a >>> forcibly-fatal SIGSYS? That way, the parent's waitid() / SIGCHLD >>> would indicate CLD_KILLED with si_status == SIGSYS. The parent could >>> check that and report that the child was probably compromised. >>> >>> --Andy >> >> I'd prefer sticking with do_exit. This provides much less chance of >> things going wrong. A parent seeing a child killed with SIGKILL is >> already pretty distinct, IMO. > > Hrm, it might be possible to do_exit(SIGSYS) which would be both. It > looks like tsk->exit_code would be SIGSYS then, but I'll look a little > more closely to see what that'll actually do. As long as there's no way it can get blocked, I'd be fine with that. It would, actually, be better than SIGKILL because, as Andy said, it's more distinguishable from other situations. I've long wanted a signal to be used for "violated policy" that wasn't just a straight SIGKILL. -Kees
On Wed, Feb 22, 2012 at 3:38 PM, Andrew Lutomirski <luto@mit.edu> wrote: > I wonder if it would be helpful to change the semantics of RET_KILL > slightly. Rather than killing via do_exit, what if it killed via a > forcibly-fatal SIGSYS? That way, the parent's waitid() / SIGCHLD > would indicate CLD_KILLED with si_status == SIGSYS. The parent could > check that and report that the child was probably compromised. That would be better. But it is certainly a more complex code path, which makes the security weenies twitch. As to concrete issues, any "normal" path needs the changes that are maybe pending from Oleg to make it actually abort the syscall instead of completing it before getting to the signal path. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/22/2012 04:08 PM, Kees Cook wrote: >> >> Hrm, it might be possible to do_exit(SIGSYS) which would be both. It >> looks like tsk->exit_code would be SIGSYS then, but I'll look a little >> more closely to see what that'll actually do. > > As long as there's no way it can get blocked, I'd be fine with that. > It would, actually, be better than SIGKILL because, as Andy said, it's > more distinguishable from other situations. I've long wanted a signal > to be used for "violated policy" that wasn't just a straight SIGKILL. > Can we really introduce force-kill semantics for a POSIX-defined signal? Other user space programs might use it for other purposes. I'm wondering if the right thing may be to introduce some variant of exit() which can return more information about a signal, including some kind of cause code for SIGKILL? -hpa
On Wed, Feb 22, 2012 at 4:29 PM, H. Peter Anvin <hpa@zytor.com> wrote: > Can we really introduce force-kill semantics for a POSIX-defined signal? > Other user space programs might use it for other purposes. The semantics are based on how the signal was generated, not what signal number it was. The only thing that depends on the signal number is SYNCHRONOUS_MASK, which just determines in which order pending signals are dequeued (POSIX says it may be any order). We only have that so your state doesn't get unhelpfully warped to another signal handler entry point (including fiddling the stack) before you dump core. No use of SIGSYS is specified by POSIX at all, of course, since "system call" is an implementation concept below the level POSIX specifies. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/22/2012 04:50 PM, Roland McGrath wrote: > On Wed, Feb 22, 2012 at 4:29 PM, H. Peter Anvin <hpa@zytor.com> wrote: >> Can we really introduce force-kill semantics for a POSIX-defined signal? >> Other user space programs might use it for other purposes. > > The semantics are based on how the signal was generated, not what signal > number it was. The only thing that depends on the signal number is > SYNCHRONOUS_MASK, which just determines in which order pending signals are > dequeued (POSIX says it may be any order). We only have that so your state > doesn't get unhelpfully warped to another signal handler entry point > (including fiddling the stack) before you dump core. > > No use of SIGSYS is specified by POSIX at all, of course, since "system > call" is an implementation concept below the level POSIX specifies. I meant whether or not a signal can be blocked/caught and the fact that the signal exists at all. Now I guess we could have "blockable" and "unblockable" SIGSYS, but that would seem to have its own set of issues... -hpa
On Wed, Feb 22, 2012 at 6:29 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 02/22/2012 04:08 PM, Kees Cook wrote: >>> >>> Hrm, it might be possible to do_exit(SIGSYS) which would be both. It >>> looks like tsk->exit_code would be SIGSYS then, but I'll look a little >>> more closely to see what that'll actually do. >> >> As long as there's no way it can get blocked, I'd be fine with that. >> It would, actually, be better than SIGKILL because, as Andy said, it's >> more distinguishable from other situations. I've long wanted a signal >> to be used for "violated policy" that wasn't just a straight SIGKILL. >> > > Can we really introduce force-kill semantics for a POSIX-defined signal? > Other user space programs might use it for other purposes. > > I'm wondering if the right thing may be to introduce some variant of > exit() which can return more information about a signal, including some > kind of cause code for SIGKILL? While it'd be harder to send back extra info, passing SIGSYS to do_exit() should result in the si_status for the emitted SIGCHLD to be SIGSYS (si_status = (tsk->exit_code & 0x7f)). I think it'll still have a si_code of CLD_KILLED, but it'd be enough for a parent to differentiate the task-death path. I'll try it out before I post another patch rev. A variant that allowed extended exit information would be useful (especially for this patch series), I'm not sure I'd know where to start. cheers! will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 22, 2012 at 5:06 PM, H. Peter Anvin <hpa@zytor.com> wrote: > I meant whether or not a signal can be blocked/caught and the fact that > the signal exists at all. > > Now I guess we could have "blockable" and "unblockable" SIGSYS, but that > would seem to have its own set of issues... Oh. I certainly don't think we should ever add any new signals to the set that cannot be caught, blocked, or ignored. That has been just SIGKILL and SIGSTOP since 4.2BSD, which first introduced the modern concept of blocking signals. There are lots of reasons not to change that, which I won't go into unless someone really wants me to. However, I don't think there is anything really wrong with having certain cases that generate a signal and at the same time unblock it and reset it to SIG_DFL. That's just an implementation detail of a policy of "dump core right now, no other option". (Conversely, directly calling do_exit won't ever dump core, though it can be made to look signalesque to the parent and tracers.) For seccomp-filter, I personally don't see any problem with simply generating SIGSYS in the normal way (and aborting the syscall, of course). If someone wants to ensure that SIGSYS is never caught or blocked, they can just do that by having a filter that doesn't allow it to be caught or blocked (and of course make sure to reset its inherited state). It is a bit tricky to cover all the ways, since it's not just sigaction and sigprocmask but also sigreturn, where the blocked signal set to be restored is in a slightly arcane location--but it ain't rocket science. But I don't really have any strong opinion about what seccomp-filter should do. (Though it does seem worthwhile not to rule out the possibility of dumping core on a policy violation, since that will be useful for people to debug their code.) Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 23, 2012 at 11:38 AM, Roland McGrath <mcgrathr@google.com> wrote: > On Wed, Feb 22, 2012 at 5:06 PM, H. Peter Anvin <hpa@zytor.com> wrote: >> I meant whether or not a signal can be blocked/caught and the fact that >> the signal exists at all. >> >> Now I guess we could have "blockable" and "unblockable" SIGSYS, but that >> would seem to have its own set of issues... > > Oh. I certainly don't think we should ever add any new signals to the set > that cannot be caught, blocked, or ignored. That has been just SIGKILL and > SIGSTOP since 4.2BSD, which first introduced the modern concept of blocking > signals. There are lots of reasons not to change that, which I won't go > into unless someone really wants me to. > > However, I don't think there is anything really wrong with having certain > cases that generate a signal and at the same time unblock it and reset it > to SIG_DFL. That's just an implementation detail of a policy of "dump core > right now, no other option". (Conversely, directly calling do_exit won't > ever dump core, though it can be made to look signalesque to the parent and > tracers.) > > For seccomp-filter, I personally don't see any problem with simply > generating SIGSYS in the normal way (and aborting the syscall, of course). > If someone wants to ensure that SIGSYS is never caught or blocked, they can > just do that by having a filter that doesn't allow it to be caught or > blocked (and of course make sure to reset its inherited state). It is a > bit tricky to cover all the ways, since it's not just sigaction and > sigprocmask but also sigreturn, where the blocked signal set to be restored > is in a slightly arcane location--but it ain't rocket science. > > But I don't really have any strong opinion about what seccomp-filter should > do. (Though it does seem worthwhile not to rule out the possibility of > dumping core on a policy violation, since that will be useful for people to > debug their code.) Seems like there's an argument for another return code, SECCOMP_RET_CORE, that resets/unblocks the SIGSYS handler since the existing TRAP and KILL options seem to cover the other paths (signal handler and do_exit). It's a very small tweak if that'd be useful to include explicitly. Thanks! will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, February 23, 2012 20:26, Will Drewry wrote: > Seems like there's an argument for another return code, > SECCOMP_RET_CORE, that resets/unblocks the SIGSYS handler since the > existing TRAP and KILL options seem to cover the other paths (signal > handler and do_exit). What about making SECCOMP_RET_TRAP dump core/send SIGSYS if there is no tracer with PTRACE_O_SECCOMP set? And perhaps go for a blockable SIGSYS? That way you only have KILL, ERRNO and TRAP, with the last one meaning deny, but giving someone else a chance to do something. Or is that just confusing? I don't think there should be too many return values, or else you put too much runtime policy into the filters. Sending SIGSYS is useful, but it's quite a bit less useful if user space can't handle it in a signal handler, so I don't think it's worth it to make a unblockable version. Greetings, Indan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 23, 2012 at 14:15, Indan Zupancic <indan@nul.nu> wrote: > What about making SECCOMP_RET_TRAP dump core/send SIGSYS if there is > no tracer with PTRACE_O_SECCOMP set? Please don't make things dependent on having a tracer. There are applications that don't really need a tracer; in fact, these are typically the exact same applications that can benefit from receiving SIGSYS and then handling it internally. If a tracer was required to set this up, it would make it difficult to use gdb, strace, or any other common debugging tools. > Sending SIGSYS is useful, but it's quite a bit less useful if user > space can't handle it in a signal handler, so I don't think it's > worth it to make a unblockable version. Maybe, I am not parsing your e-mail correctly. But don't we already get the desired behavior, if SIGSYS is treated the same as any other synchronous signal? If it is unblocked and has a handler, the application can decide to handle it. If neither one of these conditions is true, it terminates the program. Ulimits and PR_SET_DUMPABLE determine whether a core file is generated. Markus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 23, 2012 at 4:15 PM, Indan Zupancic <indan@nul.nu> wrote: > On Thu, February 23, 2012 20:26, Will Drewry wrote: >> Seems like there's an argument for another return code, >> SECCOMP_RET_CORE, that resets/unblocks the SIGSYS handler since the >> existing TRAP and KILL options seem to cover the other paths (signal >> handler and do_exit). > > What about making SECCOMP_RET_TRAP dump core/send SIGSYS if there is > no tracer with PTRACE_O_SECCOMP set? And perhaps go for a blockable > SIGSYS? That way you only have KILL, ERRNO and TRAP, with the last > one meaning deny, but giving someone else a chance to do something. > Or is that just confusing? I don't think it makes sense to mix up signal delivery for in-process handling and ptrace. In particular, TRACE calls must assume t the ptracer actually enacted a policy, but with TRAP as is, it always rejects it. > I don't think there should be too many return values, or else you > put too much runtime policy into the filters. I'd rather make it explicit than not. This will be a quagmire if any behavior is implicit. > Sending SIGSYS is useful, but it's quite a bit less useful if user > space can't handle it in a signal handler, so I don't think it's > worth it to make a unblockable version. I believe the point here would be that you'd get a useful coredump without needing to enforce that the process can't handle normal SIGSYS or other syscalls by blocking signal masking. cheers! will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 23, 2012 at 4:33 PM, Markus Gutschke <markus@chromium.org> wrote: > On Thu, Feb 23, 2012 at 14:15, Indan Zupancic <indan@nul.nu> wrote: >> What about making SECCOMP_RET_TRAP dump core/send SIGSYS if there is >> no tracer with PTRACE_O_SECCOMP set? > > Please don't make things dependent on having a tracer. There are > applications that don't really need a tracer; in fact, these are > typically the exact same applications that can benefit from receiving > SIGSYS and then handling it internally. > > If a tracer was required to set this up, it would make it difficult to > use gdb, strace, or any other common debugging tools. > >> Sending SIGSYS is useful, but it's quite a bit less useful if user >> space can't handle it in a signal handler, so I don't think it's >> worth it to make a unblockable version. > > Maybe, I am not parsing your e-mail correctly. But don't we already > get the desired behavior, if SIGSYS is treated the same as any other > synchronous signal? If it is unblocked and has a handler, the > application can decide to handle it. If neither one of these > conditions is true, it terminates the program. Ulimits and > PR_SET_DUMPABLE determine whether a core file is generated. Yeah - the current patchset does that just fine. The tweak I was proposing was making ti possible to deliver an SIGSYS that always uses SIG_DFL so that you don't have to play with signal call enforcement in the filters. This is a pretty minor tweak either way. cheers! will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, February 23, 2012 23:33, Markus Gutschke wrote: > On Thu, Feb 23, 2012 at 14:15, Indan Zupancic <indan@nul.nu> wrote: >> What about making SECCOMP_RET_TRAP dump core/send SIGSYS if there is >> no tracer with PTRACE_O_SECCOMP set? > > Please don't make things dependent on having a tracer. There are > applications that don't really need a tracer; in fact, these are > typically the exact same applications that can benefit from receiving > SIGSYS and then handling it internally. My proposal was to send the SIGSYS only when there is no seccomp aware tracer. If there is no such tracer, the process will receive a SIGSYS that it can handle internally. So having a tracer isn't required. I'm curious how you would like to handle SIGSYSs internally, because I don't see how you could gracefully recover from such failed system call, so I don't really see the added value compared to fail the syscall with some ERRNO or to just kill the task. Is it just for notification purposes? > > If a tracer was required to set this up, it would make it difficult to > use gdb, strace, or any other common debugging tools. gdb and strace and such won't set the PTRACE_O_SECCOMP option, so it will behave the same whether it's being debugged or not. The main objective was to reduce the amount of policy in filters, I thought it could be done by having only one return value which delegates to user space. But that may be too confusing, and the interaction between a seccomp aware tracer and SIGSYS aware code is fuzzy, so I'm not sure if it's a good idea. > >> Sending SIGSYS is useful, but it's quite a bit less useful if user >> space can't handle it in a signal handler, so I don't think it's >> worth it to make a unblockable version. > > Maybe, I am not parsing your e-mail correctly. But don't we already > get the desired behavior, if SIGSYS is treated the same as any other > synchronous signal? If it is unblocked and has a handler, the > application can decide to handle it. If neither one of these > conditions is true, it terminates the program. Ulimits and > PR_SET_DUMPABLE determine whether a core file is generated. The proposal I was replying to wanted to make SIGSYS always kill the process (with a core dump), so you wouldn't be able to set a handler any more. I think that is a bad idea. Or did I misunderstood? Enforcing task termination when there is no handler doesn't make conceptual sense, because an empty signal handler is effectively the same as blocking a signal. Though I guess it's simpler to check for just sigaction in the BPF filters, so perhaps that was the idea. Greetings, Indan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 27, 2012 at 6:32 AM, Indan Zupancic <indan@nul.nu> wrote: > On Thu, February 23, 2012 23:33, Markus Gutschke wrote: >> On Thu, Feb 23, 2012 at 14:15, Indan Zupancic <indan@nul.nu> wrote: >>> What about making SECCOMP_RET_TRAP dump core/send SIGSYS if there is >>> no tracer with PTRACE_O_SECCOMP set? >> >> Please don't make things dependent on having a tracer. There are >> applications that don't really need a tracer; in fact, these are >> typically the exact same applications that can benefit from receiving >> SIGSYS and then handling it internally. > > My proposal was to send the SIGSYS only when there is no seccomp aware > tracer. If there is no such tracer, the process will receive a SIGSYS > that it can handle internally. So having a tracer isn't required. > > I'm curious how you would like to handle SIGSYSs internally, because I > don't see how you could gracefully recover from such failed system call, > so I don't really see the added value compared to fail the syscall with > some ERRNO or to just kill the task. Is it just for notification purposes? Take a look at samples/seccomp/bpf-direct.c. You can emulate the call which can be useful for patching up external code. This is especially useful if you don't want to hand patch every library that is doing something you don't like without a full supervisor framework (e.g., glibc checking for an nscd.socket file). Another use is implementing a signal-handler based system call delegation system. E.g., setup your fds with a broker, then pass the requested syscall number and desired arguments over to the broker from the hanlder who can pass back an fd or whatever is appropriate. Then the return from the syscall can be fixed up. >> >> If a tracer was required to set this up, it would make it difficult to >> use gdb, strace, or any other common debugging tools. > > gdb and strace and such won't set the PTRACE_O_SECCOMP option, so it > will behave the same whether it's being debugged or not. > > The main objective was to reduce the amount of policy in filters, > I thought it could be done by having only one return value which > delegates to user space. But that may be too confusing, and the > interaction between a seccomp aware tracer and SIGSYS aware code > is fuzzy, so I'm not sure if it's a good idea. Yeah - I want to avoid as much implicit behavior as possible. It's a trap I regularly fall in and both you and luto@ have kept me honest this time. I don't want to regress :) >> >>> Sending SIGSYS is useful, but it's quite a bit less useful if user >>> space can't handle it in a signal handler, so I don't think it's >>> worth it to make a unblockable version. >> >> Maybe, I am not parsing your e-mail correctly. But don't we already >> get the desired behavior, if SIGSYS is treated the same as any other >> synchronous signal? If it is unblocked and has a handler, the >> application can decide to handle it. If neither one of these >> conditions is true, it terminates the program. Ulimits and >> PR_SET_DUMPABLE determine whether a core file is generated. > > The proposal I was replying to wanted to make SIGSYS always kill the > process (with a core dump), so you wouldn't be able to set a handler > any more. I think that is a bad idea. Or did I misunderstood? Yeah - I suspect some crossed wires. The idea of a forced core dump seems useful in some scenarios, but it is not hard to synthesize with the RET_TRAP already. If something like RET_CORE is more obviously useful after we have all the proposed consumers using this interface, then it would make sense to entertain it then. > Enforcing task termination when there is no handler doesn't make > conceptual sense, because an empty signal handler is effectively > the same as blocking a signal. Though I guess it's simpler to check > for just sigaction in the BPF filters, so perhaps that was the idea. Pretty much, I guess. Right now RET_TRAP calls force_siginfo which will either use an installed handler or unblock the signal and use SIG_DFL, which is just dump core. So the tweak would be to just set it back to SIG_DFL prior to delivery (like force_sigsegv does when it detects it is double-faulting). Anyway, I think that forcing a coredump with a return code is overkill at this point. do_exit(SIGSYS) is nice and so is using RET_TRAP to trigger a core. Whether RET_TRACE should emit a sigsys when a tracer isn't present is less clear to me, but I think keeping behavior explicit will end up leading to the least number of mistakes and breaks. Thanks! will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c index 6557769..c81d2c7 100644 --- a/arch/x86/ia32/ia32_signal.c +++ b/arch/x86/ia32/ia32_signal.c @@ -73,6 +73,10 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, siginfo_t *from) switch (from->si_code >> 16) { case __SI_FAULT >> 16: break; + case __SI_SYS >> 16: + put_user_ex(from->si_syscall, &to->si_syscall); + put_user_ex(from->si_arch, &to->si_arch); + break; case __SI_CHLD >> 16: put_user_ex(from->si_utime, &to->si_utime); put_user_ex(from->si_stime, &to->si_stime); diff --git a/arch/x86/include/asm/ia32.h b/arch/x86/include/asm/ia32.h index 1f7e625..541485f 100644 --- a/arch/x86/include/asm/ia32.h +++ b/arch/x86/include/asm/ia32.h @@ -126,6 +126,12 @@ typedef struct compat_siginfo { int _band; /* POLL_IN, POLL_OUT, POLL_MSG */ int _fd; } _sigpoll; + + struct { + unsigned int _call_addr; /* calling insn */ + int _syscall; /* triggering system call number */ + unsigned int _arch; /* AUDIT_ARCH_* of syscall */ + } _sigsys; } _sifields; } compat_siginfo_t; diff --git a/include/asm-generic/siginfo.h b/include/asm-generic/siginfo.h index 0dd4e87..a83b478 100644 --- a/include/asm-generic/siginfo.h +++ b/include/asm-generic/siginfo.h @@ -90,6 +90,13 @@ typedef struct siginfo { __ARCH_SI_BAND_T _band; /* POLL_IN, POLL_OUT, POLL_MSG */ int _fd; } _sigpoll; + + /* SIGSYS */ + struct { + void __user *_call_addr; /* calling insn */ + int _syscall; /* triggering system call number */ + unsigned int _arch; /* AUDIT_ARCH_* of syscall */ + } _sigsys; } _sifields; } siginfo_t; @@ -116,6 +123,9 @@ typedef struct siginfo { #define si_addr_lsb _sifields._sigfault._addr_lsb #define si_band _sifields._sigpoll._band #define si_fd _sifields._sigpoll._fd +#define si_call_addr _sifields._sigsys._call_addr +#define si_syscall _sifields._sigsys._syscall +#define si_arch _sifields._sigsys._arch #ifdef __KERNEL__ #define __SI_MASK 0xffff0000u @@ -126,6 +136,7 @@ typedef struct siginfo { #define __SI_CHLD (4 << 16) #define __SI_RT (5 << 16) #define __SI_MESGQ (6 << 16) +#define __SI_SYS (7 << 16) #define __SI_CODE(T,N) ((T) | ((N) & 0xffff)) #else #define __SI_KILL 0 @@ -135,6 +146,7 @@ typedef struct siginfo { #define __SI_CHLD 0 #define __SI_RT 0 #define __SI_MESGQ 0 +#define __SI_SYS 0 #define __SI_CODE(T,N) (N) #endif @@ -232,6 +244,12 @@ typedef struct siginfo { #define NSIGPOLL 6 /* + * SIGSYS si_codes + */ +#define SYS_SECCOMP (__SI_SYS|1) /* seccomp triggered */ +#define NSIGSYS 1 + +/* * sigevent definitions * * It seems likely that SIGEV_THREAD will have to be handled from diff --git a/kernel/signal.c b/kernel/signal.c index c73c428..7573819 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -160,7 +160,7 @@ void recalc_sigpending(void) #define SYNCHRONOUS_MASK \ (sigmask(SIGSEGV) | sigmask(SIGBUS) | sigmask(SIGILL) | \ - sigmask(SIGTRAP) | sigmask(SIGFPE)) + sigmask(SIGTRAP) | sigmask(SIGFPE) | sigmask(SIGSYS)) int next_signal(struct sigpending *pending, sigset_t *mask) {
This change enables SIGSYS, defines _sigfields._sigsys, and adds x86 (compat) arch support. _sigsys defines fields which allow a signal handler to receive the triggering system call number, the relevant AUDIT_ARCH_* value for that number, and the address of the callsite. To ensure that SIGSYS delivery occurs on return from the triggering system call, SIGSYS is added to the SYNCHRONOUS_MASK macro. I'm this is enough to ensure it will be synchronous or if it is explicitly required to ensure an immediate delivery of the signal upon return from the blocked system call. The first consumer of SIGSYS would be seccomp filter. In particular, a filter program could specify a new return value, SECCOMP_RET_TRAP, which would result in the system call being denied and the calling thread signaled. This also means that implementing arch-specific support can be dependent upon HAVE_ARCH_SECCOMP_FILTER. v10: - first version based on suggestion Suggested-by: H. Peter Anvin <hpa@zytor.com> Signed-off-by: Will Drewry <wad@chromium.org> --- arch/x86/ia32/ia32_signal.c | 4 ++++ arch/x86/include/asm/ia32.h | 6 ++++++ include/asm-generic/siginfo.h | 18 ++++++++++++++++++ kernel/signal.c | 2 +- 4 files changed, 29 insertions(+), 1 deletions(-)