Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 22 Feb 2012 13:48:08 -0600
From: Will Drewry <wad@...omium.org>
To: Indan Zupancic <indan@....nu>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org, 
	linux-doc@...r.kernel.org, kernel-hardening@...ts.openwall.com, 
	netdev@...r.kernel.org, x86@...nel.org, arnd@...db.de, davem@...emloft.net, 
	hpa@...or.com, mingo@...hat.com, oleg@...hat.com, peterz@...radead.org, 
	rdunlap@...otime.net, mcgrathr@...omium.org, tglx@...utronix.de, luto@....edu, 
	eparis@...hat.com, serge.hallyn@...onical.com, djm@...drot.org, 
	scarybeasts@...il.com, pmoore@...hat.com, akpm@...ux-foundation.org, 
	corbet@....net, eric.dumazet@...il.com, markus@...omium.org, 
	keescook@...omium.org
Subject: Re: [PATCH v10 07/11] signal, x86: add SIGSYS info and make it synchronous.

On Wed, Feb 22, 2012 at 2:34 AM, Indan Zupancic <indan@....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@...or.com>
>> Signed-off-by: Will Drewry <wad@...omium.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

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.