| Message ID | 20120226202828.GE3990@outflux.net |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, Feb 26, 2012 at 2:28 PM, Kees Cook <kees@ubuntu.com> wrote: > On Fri, Feb 24, 2012 at 09:21:45PM -0600, Will Drewry wrote: >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> index e8d76c5..25e8296 100644 >> --- a/kernel/seccomp.c >> +++ b/kernel/seccomp.c >> [...] >> +static void seccomp_filter_log_failure(int syscall) >> +{ >> + int compat = 0; >> +#ifdef CONFIG_COMPAT >> + compat = is_compat_task(); >> +#endif >> + pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n", >> + current->comm, task_pid_nr(current), >> + (compat ? "compat " : ""), >> + syscall, KSTK_EIP(current)); >> +} >> [...] >> +#ifdef CONFIG_SECCOMP_FILTER >> + case SECCOMP_MODE_FILTER: >> + if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW) >> + return; >> + seccomp_filter_log_failure(this_syscall); >> + exit_code = SIGSYS; >> + break; >> +#endif >> default: >> BUG(); >> } >> @@ -56,7 +324,7 @@ void __secure_computing(int this_syscall) >> dump_stack(); >> #endif >> audit_seccomp(this_syscall); >> - do_exit(SIGKILL); >> + do_exit(exit_code); >> } > > I think the seccomp_filter_log_failure() use is redundant with the > audit_seccomp call. Here's a possible reorganization of the logging... Cool - a comment below: > From: Kees Cook <keescook@chromium.org> > Date: Sun, 26 Feb 2012 11:56:12 -0800 > Subject: [PATCH] seccomp: improve audit logging details > > This consolidates the seccomp filter error logging path and adds more > details to the audit log. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/audit.h | 8 ++++---- > kernel/auditsc.c | 9 +++++++-- > kernel/seccomp.c | 15 +-------------- > 3 files changed, 12 insertions(+), 20 deletions(-) > > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 9ff7a2c..5aa6cfc 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -463,7 +463,7 @@ extern void audit_putname(const char *name); > extern void __audit_inode(const char *name, const struct dentry *dentry); > extern void __audit_inode_child(const struct dentry *dentry, > const struct inode *parent); > -extern void __audit_seccomp(unsigned long syscall); > +extern void __audit_seccomp(unsigned long syscall, long signr); > extern void __audit_ptrace(struct task_struct *t); > > static inline int audit_dummy_context(void) > @@ -508,10 +508,10 @@ static inline void audit_inode_child(const struct dentry *dentry, > } > void audit_core_dumps(long signr); > > -static inline void audit_seccomp(unsigned long syscall) > +static inline void audit_seccomp(unsigned long syscall, long signr) > { > if (unlikely(!audit_dummy_context())) > - __audit_seccomp(syscall); > + __audit_seccomp(syscall, signr); > } > > static inline void audit_ptrace(struct task_struct *t) > @@ -634,7 +634,7 @@ extern int audit_signals; > #define audit_inode(n,d) do { (void)(d); } while (0) > #define audit_inode_child(i,p) do { ; } while (0) > #define audit_core_dumps(i) do { ; } while (0) > -#define audit_seccomp(i) do { ; } while (0) > +#define audit_seccomp(i,s) do { ; } while (0) > #define auditsc_get_stamp(c,t,s) (0) > #define audit_get_loginuid(t) (-1) > #define audit_get_sessionid(t) (-1) > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index af1de0f..74652fe 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -67,6 +67,7 @@ > #include <linux/syscalls.h> > #include <linux/capability.h> > #include <linux/fs_struct.h> > +#include <linux/compat.h> > > #include "audit.h" > > @@ -2710,13 +2711,17 @@ void audit_core_dumps(long signr) > audit_log_end(ab); > } > > -void __audit_seccomp(unsigned long syscall) > +void __audit_seccomp(unsigned long syscall, long signr) > { > struct audit_buffer *ab; > > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND); > - audit_log_abend(ab, "seccomp", SIGKILL); > + audit_log_abend(ab, "seccomp", signr); > audit_log_format(ab, " syscall=%ld", syscall); > +#ifdef CONFIG_COMPAT > + audit_log_format(ab, " compat=%d", is_compat_task()); > +#endif Should this just use syscall_get_arch to get the AUDIT_ARCH now? :) > + audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current)); > audit_log_end(ab); > } > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index 5aabc3c..40af83f 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -57,18 +57,6 @@ struct seccomp_filter { > struct sock_filter insns[]; > }; > > -static void seccomp_filter_log_failure(int syscall) > -{ > - int compat = 0; > -#ifdef CONFIG_COMPAT > - compat = is_compat_task(); > -#endif > - pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n", > - current->comm, task_pid_nr(current), > - (compat ? "compat " : ""), > - syscall, KSTK_EIP(current)); > -} > - > /** > * get_u32 - returns a u32 offset into data > * @data: a unsigned 64 bit value > @@ -378,7 +366,6 @@ int __secure_computing_int(int this_syscall) > default: > break; > } > - seccomp_filter_log_failure(this_syscall); > exit_code = SIGSYS; > break; > } > @@ -390,7 +377,7 @@ int __secure_computing_int(int this_syscall) > #ifdef SECCOMP_DEBUG > dump_stack(); > #endif > - audit_seccomp(this_syscall); > + audit_seccomp(this_syscall, exit_code); > do_exit(exit_code); > return -1; /* never reached */ > } > -- > 1.7.0.4 I'll pull this into the series if that's okay with you? Thanks! -- 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, 2012-02-27 at 10:23 -0600, Will Drewry wrote: > On Sun, Feb 26, 2012 at 2:28 PM, Kees Cook <kees@ubuntu.com> wrote: > > On Fri, Feb 24, 2012 at 09:21:45PM -0600, Will Drewry wrote: > >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c > >> index e8d76c5..25e8296 100644 > >> --- a/kernel/seccomp.c > >> +++ b/kernel/seccomp.c > >> [...] > >> +static void seccomp_filter_log_failure(int syscall) > >> +{ > >> + int compat = 0; > >> +#ifdef CONFIG_COMPAT > >> + compat = is_compat_task(); > >> +#endif > >> + pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n", > >> + current->comm, task_pid_nr(current), > >> + (compat ? "compat " : ""), > >> + syscall, KSTK_EIP(current)); > >> +} > >> [...] > >> +#ifdef CONFIG_SECCOMP_FILTER > >> + case SECCOMP_MODE_FILTER: > >> + if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW) > >> + return; > >> + seccomp_filter_log_failure(this_syscall); > >> + exit_code = SIGSYS; > >> + break; > >> +#endif > >> default: > >> BUG(); > >> } > >> @@ -56,7 +324,7 @@ void __secure_computing(int this_syscall) > >> dump_stack(); > >> #endif > >> audit_seccomp(this_syscall); > >> - do_exit(SIGKILL); > >> + do_exit(exit_code); > >> } > > > > I think the seccomp_filter_log_failure() use is redundant with the > > audit_seccomp call. Here's a possible reorganization of the logging... > > Cool - a comment below: > > > From: Kees Cook <keescook@chromium.org> > > Date: Sun, 26 Feb 2012 11:56:12 -0800 > > Subject: [PATCH] seccomp: improve audit logging details > > > > This consolidates the seccomp filter error logging path and adds more > > details to the audit log. > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > include/linux/audit.h | 8 ++++---- > > kernel/auditsc.c | 9 +++++++-- > > kernel/seccomp.c | 15 +-------------- > > 3 files changed, 12 insertions(+), 20 deletions(-) > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 9ff7a2c..5aa6cfc 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -463,7 +463,7 @@ extern void audit_putname(const char *name); > > extern void __audit_inode(const char *name, const struct dentry *dentry); > > extern void __audit_inode_child(const struct dentry *dentry, > > const struct inode *parent); > > -extern void __audit_seccomp(unsigned long syscall); > > +extern void __audit_seccomp(unsigned long syscall, long signr); > > extern void __audit_ptrace(struct task_struct *t); > > > > static inline int audit_dummy_context(void) > > @@ -508,10 +508,10 @@ static inline void audit_inode_child(const struct dentry *dentry, > > } > > void audit_core_dumps(long signr); > > > > -static inline void audit_seccomp(unsigned long syscall) > > +static inline void audit_seccomp(unsigned long syscall, long signr) > > { > > if (unlikely(!audit_dummy_context())) > > - __audit_seccomp(syscall); > > + __audit_seccomp(syscall, signr); > > } > > > > static inline void audit_ptrace(struct task_struct *t) > > @@ -634,7 +634,7 @@ extern int audit_signals; > > #define audit_inode(n,d) do { (void)(d); } while (0) > > #define audit_inode_child(i,p) do { ; } while (0) > > #define audit_core_dumps(i) do { ; } while (0) > > -#define audit_seccomp(i) do { ; } while (0) > > +#define audit_seccomp(i,s) do { ; } while (0) > > #define auditsc_get_stamp(c,t,s) (0) > > #define audit_get_loginuid(t) (-1) > > #define audit_get_sessionid(t) (-1) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index af1de0f..74652fe 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -67,6 +67,7 @@ > > #include <linux/syscalls.h> > > #include <linux/capability.h> > > #include <linux/fs_struct.h> > > +#include <linux/compat.h> > > > > #include "audit.h" > > > > @@ -2710,13 +2711,17 @@ void audit_core_dumps(long signr) > > audit_log_end(ab); > > } > > > > -void __audit_seccomp(unsigned long syscall) > > +void __audit_seccomp(unsigned long syscall, long signr) > > { > > struct audit_buffer *ab; > > > > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND); > > - audit_log_abend(ab, "seccomp", SIGKILL); > > + audit_log_abend(ab, "seccomp", signr); > > audit_log_format(ab, " syscall=%ld", syscall); > > +#ifdef CONFIG_COMPAT > > + audit_log_format(ab, " compat=%d", is_compat_task()); > > +#endif > > Should this just use syscall_get_arch to get the AUDIT_ARCH now? :) I'm waffling on this one, but I'm leaning towards not including compat at all. If you include it, yes, you should use the generic function. If you have CONFIG_AUDITSC and started audit you are going to get this, along with a0-a4, in a separate but associated audit record. Thus you get all the interesting/relevant info. Without CONFIG_AUDITSC and running auditd you get compat, but nothing else. Why is compat so interesting? This patch would duplicate the arch=field from that record (calling it compat). So if we are going to duplicate it in another record, we should at least call it the same thing (arch=%x) My current thinking, and I'm not settled would be to include syscall, a0-a4 and arch in the record only if !CONFIG_AUDITSC. (ip doesn't show up elsewhere, so that makes sense here) It might be annoying to have to find the info in the right record, but if you use the auparse audit library tools, it should 'Just Work'... > > + audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current)); > > audit_log_end(ab); > > } > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > index 5aabc3c..40af83f 100644 > > --- a/kernel/seccomp.c > > +++ b/kernel/seccomp.c > > @@ -57,18 +57,6 @@ struct seccomp_filter { > > struct sock_filter insns[]; > > }; > > > > -static void seccomp_filter_log_failure(int syscall) > > -{ > > - int compat = 0; > > -#ifdef CONFIG_COMPAT > > - compat = is_compat_task(); > > -#endif > > - pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n", > > - current->comm, task_pid_nr(current), > > - (compat ? "compat " : ""), > > - syscall, KSTK_EIP(current)); > > -} > > - > > /** > > * get_u32 - returns a u32 offset into data > > * @data: a unsigned 64 bit value > > @@ -378,7 +366,6 @@ int __secure_computing_int(int this_syscall) > > default: > > break; > > } > > - seccomp_filter_log_failure(this_syscall); > > exit_code = SIGSYS; > > break; > > } > > @@ -390,7 +377,7 @@ int __secure_computing_int(int this_syscall) > > #ifdef SECCOMP_DEBUG > > dump_stack(); > > #endif > > - audit_seccomp(this_syscall); > > + audit_seccomp(this_syscall, exit_code); > > do_exit(exit_code); > > return -1; /* never reached */ > > } > > -- > > 1.7.0.4 > > I'll pull this into the series if that's okay with you? > > Thanks! -- 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 8:49 AM, Eric Paris <eparis@redhat.com> wrote: > On Mon, 2012-02-27 at 10:23 -0600, Will Drewry wrote: >> On Sun, Feb 26, 2012 at 2:28 PM, Kees Cook <kees@ubuntu.com> wrote: >> > On Fri, Feb 24, 2012 at 09:21:45PM -0600, Will Drewry wrote: >> >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> >> index e8d76c5..25e8296 100644 >> >> --- a/kernel/seccomp.c >> >> +++ b/kernel/seccomp.c >> >> [...] >> >> +static void seccomp_filter_log_failure(int syscall) >> >> +{ >> >> + int compat = 0; >> >> +#ifdef CONFIG_COMPAT >> >> + compat = is_compat_task(); >> >> +#endif >> >> + pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n", >> >> + current->comm, task_pid_nr(current), >> >> + (compat ? "compat " : ""), >> >> + syscall, KSTK_EIP(current)); >> >> +} >> >> [...] >> >> +#ifdef CONFIG_SECCOMP_FILTER >> >> + case SECCOMP_MODE_FILTER: >> >> + if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW) >> >> + return; >> >> + seccomp_filter_log_failure(this_syscall); >> >> + exit_code = SIGSYS; >> >> + break; >> >> +#endif >> >> default: >> >> BUG(); >> >> } >> >> @@ -56,7 +324,7 @@ void __secure_computing(int this_syscall) >> >> dump_stack(); >> >> #endif >> >> audit_seccomp(this_syscall); >> >> - do_exit(SIGKILL); >> >> + do_exit(exit_code); >> >> } >> > >> > I think the seccomp_filter_log_failure() use is redundant with the >> > audit_seccomp call. Here's a possible reorganization of the logging... >> >> Cool - a comment below: >> >> > From: Kees Cook <keescook@chromium.org> >> > Date: Sun, 26 Feb 2012 11:56:12 -0800 >> > Subject: [PATCH] seccomp: improve audit logging details >> > >> > This consolidates the seccomp filter error logging path and adds more >> > details to the audit log. >> > >> > Signed-off-by: Kees Cook <keescook@chromium.org> >> > --- >> > include/linux/audit.h | 8 ++++---- >> > kernel/auditsc.c | 9 +++++++-- >> > kernel/seccomp.c | 15 +-------------- >> > 3 files changed, 12 insertions(+), 20 deletions(-) >> > >> > diff --git a/include/linux/audit.h b/include/linux/audit.h >> > index 9ff7a2c..5aa6cfc 100644 >> > --- a/include/linux/audit.h >> > +++ b/include/linux/audit.h >> > @@ -463,7 +463,7 @@ extern void audit_putname(const char *name); >> > extern void __audit_inode(const char *name, const struct dentry *dentry); >> > extern void __audit_inode_child(const struct dentry *dentry, >> > const struct inode *parent); >> > -extern void __audit_seccomp(unsigned long syscall); >> > +extern void __audit_seccomp(unsigned long syscall, long signr); >> > extern void __audit_ptrace(struct task_struct *t); >> > >> > static inline int audit_dummy_context(void) >> > @@ -508,10 +508,10 @@ static inline void audit_inode_child(const struct dentry *dentry, >> > } >> > void audit_core_dumps(long signr); >> > >> > -static inline void audit_seccomp(unsigned long syscall) >> > +static inline void audit_seccomp(unsigned long syscall, long signr) >> > { >> > if (unlikely(!audit_dummy_context())) >> > - __audit_seccomp(syscall); >> > + __audit_seccomp(syscall, signr); >> > } >> > >> > static inline void audit_ptrace(struct task_struct *t) >> > @@ -634,7 +634,7 @@ extern int audit_signals; >> > #define audit_inode(n,d) do { (void)(d); } while (0) >> > #define audit_inode_child(i,p) do { ; } while (0) >> > #define audit_core_dumps(i) do { ; } while (0) >> > -#define audit_seccomp(i) do { ; } while (0) >> > +#define audit_seccomp(i,s) do { ; } while (0) >> > #define auditsc_get_stamp(c,t,s) (0) >> > #define audit_get_loginuid(t) (-1) >> > #define audit_get_sessionid(t) (-1) >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> > index af1de0f..74652fe 100644 >> > --- a/kernel/auditsc.c >> > +++ b/kernel/auditsc.c >> > @@ -67,6 +67,7 @@ >> > #include <linux/syscalls.h> >> > #include <linux/capability.h> >> > #include <linux/fs_struct.h> >> > +#include <linux/compat.h> >> > >> > #include "audit.h" >> > >> > @@ -2710,13 +2711,17 @@ void audit_core_dumps(long signr) >> > audit_log_end(ab); >> > } >> > >> > -void __audit_seccomp(unsigned long syscall) >> > +void __audit_seccomp(unsigned long syscall, long signr) >> > { >> > struct audit_buffer *ab; >> > >> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND); >> > - audit_log_abend(ab, "seccomp", SIGKILL); >> > + audit_log_abend(ab, "seccomp", signr); >> > audit_log_format(ab, " syscall=%ld", syscall); >> > +#ifdef CONFIG_COMPAT >> > + audit_log_format(ab, " compat=%d", is_compat_task()); >> > +#endif >> >> Should this just use syscall_get_arch to get the AUDIT_ARCH now? :) > > I'm waffling on this one, but I'm leaning towards not including compat > at all. If you include it, yes, you should use the generic function. > > If you have CONFIG_AUDITSC and started audit you are going to get this, > along with a0-a4, in a separate but associated audit record. Thus you > get all the interesting/relevant info. Without CONFIG_AUDITSC and > running auditd you get compat, but nothing else. Why is compat so > interesting? You mean as used in audit_log_exit() ? It looks like that depends on a lot of state cached in __audit_syscall_entry() and finally triggered in __audit_syscall_exit() (and ..._free()). I don't think this is really want seccomp wants to be involved in. By CONFIG_AUDITSC, you mean CONFIG_AUDITSYSCALL? Without that set, audit_seccomp is a no-op. The reason compat needs to be reported (or rather, arch) is because just reporting syscall is ambiguous. It either needs arch or compat to distinguish it. > This patch would duplicate the arch=field from that record (calling it > compat). So if we are going to duplicate it in another record, we > should at least call it the same thing (arch=%x) Right, I agree with Will, this should be arch=%x via syscall_get_arch() if it's going to happen here. > My current thinking, and I'm not settled would be to include syscall, > a0-a4 and arch in the record only if !CONFIG_AUDITSC. (ip doesn't show > up elsewhere, so that makes sense here) > > It might be annoying to have to find the info in the right record, but > if you use the auparse audit library tools, it should 'Just Work'... Given that this is more about logging an abend-like condition, I don't think it should need to depend on having all syscall auditing enabled for the process just to get the arch. It really feels like a distinct condition. But maybe I'm misunderstanding something about how auditsc.c does its work. >> > + audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current)); >> > audit_log_end(ab); >> > } >> > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> > index 5aabc3c..40af83f 100644 >> > --- a/kernel/seccomp.c >> > +++ b/kernel/seccomp.c >> > @@ -57,18 +57,6 @@ struct seccomp_filter { >> > struct sock_filter insns[]; >> > }; >> > >> > -static void seccomp_filter_log_failure(int syscall) >> > -{ >> > - int compat = 0; >> > -#ifdef CONFIG_COMPAT >> > - compat = is_compat_task(); >> > -#endif >> > - pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n", >> > - current->comm, task_pid_nr(current), >> > - (compat ? "compat " : ""), >> > - syscall, KSTK_EIP(current)); >> > -} >> > - >> > /** >> > * get_u32 - returns a u32 offset into data >> > * @data: a unsigned 64 bit value >> > @@ -378,7 +366,6 @@ int __secure_computing_int(int this_syscall) >> > default: >> > break; >> > } >> > - seccomp_filter_log_failure(this_syscall); >> > exit_code = SIGSYS; >> > break; >> > } >> > @@ -390,7 +377,7 @@ int __secure_computing_int(int this_syscall) >> > #ifdef SECCOMP_DEBUG >> > dump_stack(); >> > #endif >> > - audit_seccomp(this_syscall); >> > + audit_seccomp(this_syscall, exit_code); >> > do_exit(exit_code); >> > return -1; /* never reached */ >> > } >> > -- >> > 1.7.0.4 >> >> I'll pull this into the series if that's okay with you? Let me send a modified version that doesn't include arch, just to avoid that can of worms for the moment. A separate patch can add that later, along with all the get_audit_arch() routines for the other architectures. My original intent was to avoid the duplication between pr_info() and audit_seccomp(). :) -Kees
On Mon, 2012-02-27 at 10:55 -0800, Kees Cook wrote: > On Mon, Feb 27, 2012 at 8:49 AM, Eric Paris <eparis@redhat.com> wrote: > You mean as used in audit_log_exit() ? It looks like that depends on a > lot of state cached in __audit_syscall_entry() and finally triggered > in __audit_syscall_exit() (and ..._free()). I don't think this is > really want seccomp wants to be involved in. > > By CONFIG_AUDITSC, you mean CONFIG_AUDITSYSCALL? Without that set, > audit_seccomp is a no-op. > > The reason compat needs to be reported (or rather, arch) is because > just reporting syscall is ambiguous. It either needs arch or compat to > distinguish it. Yes, that is what I mean and you are right. You shouldn't push the syscall in this record either. If !audit_dummy_context() you are already going to get arch, syscall, and a0-a4 in the associated audit record. Please do not duplicate that info. It might make sense to have a separate audit_seccomp() path when audit_dummy_context() which includes arch, syscall, and a0-a4. It is my fault (85e7bac3) that we have syscall at all, but I'm on a new crusade to remove audit record duplication. So I'd happily see a patch in this series that removes that instead of adds to it. -Eric -- 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 11:25 AM, Eric Paris <eparis@redhat.com> wrote: > On Mon, 2012-02-27 at 10:55 -0800, Kees Cook wrote: >> On Mon, Feb 27, 2012 at 8:49 AM, Eric Paris <eparis@redhat.com> wrote: > >> You mean as used in audit_log_exit() ? It looks like that depends on a >> lot of state cached in __audit_syscall_entry() and finally triggered >> in __audit_syscall_exit() (and ..._free()). I don't think this is >> really want seccomp wants to be involved in. >> >> By CONFIG_AUDITSC, you mean CONFIG_AUDITSYSCALL? Without that set, >> audit_seccomp is a no-op. >> >> The reason compat needs to be reported (or rather, arch) is because >> just reporting syscall is ambiguous. It either needs arch or compat to >> distinguish it. > > Yes, that is what I mean and you are right. You shouldn't push the > syscall in this record either. If !audit_dummy_context() you are > already going to get arch, syscall, and a0-a4 in the associated audit > record. Please do not duplicate that info. Ah, in that case, please ignore the patch I just sent. Heh. > It might make sense to have a separate audit_seccomp() path when > audit_dummy_context() which includes arch, syscall, and a0-a4. Ah! I think I understand what you mean now. If audit_dummy_context(), then the syscall, arch, and a0-a4 were not already collected. Gotcha. How do you envision it looking? I still see it as two distinct events (the syscall itself, and the rejection). Would you want those details added to the context structure to be reported at ..._exit() time? It seems like context->type couldn't be used to see if those fields were valid. Something like: void __audit_seccomp(unsigned long syscall, long signr) { struct audit_buffer *ab; if (!audit_dummy_context()) { struct audit_context *context = current->audit_context; context->syscall_signr = signr; context->syscall_ip = KSTK_EIP(current); return; } ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND); audit_log_abend(ab, "seccomp", signr); audit_log_format(ab, " syscall=%ld", syscall); audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current)); audit_log_end(ab); } And then report syscall_ip and syscall_signr if syscall_signr != 0 in the _exit()? I think everything else from audit_log_abend() will end up in the _exit() report. > It is my fault (85e7bac3) that we have syscall at all, but I'm on a new > crusade to remove audit record duplication. So I'd happily see a patch > in this series that removes that instead of adds to it. Well, I think the abend reporting is nice; I'd hate to see that totally removed. The seccomp case is a bit different, I agree. I could see it either way. -Kees
On Mon, 2012-02-27 at 12:00 -0800, Kees Cook wrote: > On Mon, Feb 27, 2012 at 11:25 AM, Eric Paris <eparis@redhat.com> wrote: > > On Mon, 2012-02-27 at 10:55 -0800, Kees Cook wrote: > >> On Mon, Feb 27, 2012 at 8:49 AM, Eric Paris <eparis@redhat.com> wrote: > > > >> You mean as used in audit_log_exit() ? It looks like that depends on a > >> lot of state cached in __audit_syscall_entry() and finally triggered > >> in __audit_syscall_exit() (and ..._free()). I don't think this is > >> really want seccomp wants to be involved in. > >> > >> By CONFIG_AUDITSC, you mean CONFIG_AUDITSYSCALL? Without that set, > >> audit_seccomp is a no-op. > >> > >> The reason compat needs to be reported (or rather, arch) is because > >> just reporting syscall is ambiguous. It either needs arch or compat to > >> distinguish it. > > > > Yes, that is what I mean and you are right. You shouldn't push the > > syscall in this record either. If !audit_dummy_context() you are > > already going to get arch, syscall, and a0-a4 in the associated audit > > record. Please do not duplicate that info. > > Ah, in that case, please ignore the patch I just sent. Heh. > > > It might make sense to have a separate audit_seccomp() path when > > audit_dummy_context() which includes arch, syscall, and a0-a4. > > Ah! I think I understand what you mean now. If audit_dummy_context(), > then the syscall, arch, and a0-a4 were not already collected. Gotcha. > > How do you envision it looking? I still see it as two distinct events > (the syscall itself, and the rejection). Would you want those details > added to the context structure to be reported at ..._exit() time? It > seems like context->type couldn't be used to see if those fields were > valid. > > Something like: > > void __audit_seccomp(unsigned long syscall, long signr) > { > struct audit_buffer *ab; > > if (!audit_dummy_context()) { > struct audit_context *context = current->audit_context; > context->syscall_signr = signr; > context->syscall_ip = KSTK_EIP(current); > return; > } > > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND); > audit_log_abend(ab, "seccomp", signr); > audit_log_format(ab, " syscall=%ld", syscall); > audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current)); > audit_log_end(ab); > } > > And then report syscall_ip and syscall_signr if syscall_signr != 0 in > the _exit()? I think everything else from audit_log_abend() will end > up in the _exit() report. > > > It is my fault (85e7bac3) that we have syscall at all, but I'm on a new > > crusade to remove audit record duplication. So I'd happily see a patch > > in this series that removes that instead of adds to it. > > Well, I think the abend reporting is nice; I'd hate to see that > totally removed. The seccomp case is a bit different, I agree. I could > see it either way. Once again I send you down a bad path. Your original patch was the best. We should consider including a0-aX in a future version. I was mistaken in foolishly believing that audit_syscall_entry() was done before secure_computing(). But if you look, that isn't the case. Please pretend I never said anything as you had it right the first time. -Eric -- 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 12:34 PM, Eric Paris <eparis@redhat.com> wrote: > On Mon, 2012-02-27 at 12:00 -0800, Kees Cook wrote: >> On Mon, Feb 27, 2012 at 11:25 AM, Eric Paris <eparis@redhat.com> wrote: >> > On Mon, 2012-02-27 at 10:55 -0800, Kees Cook wrote: >> >> On Mon, Feb 27, 2012 at 8:49 AM, Eric Paris <eparis@redhat.com> wrote: >> > >> >> You mean as used in audit_log_exit() ? It looks like that depends on a >> >> lot of state cached in __audit_syscall_entry() and finally triggered >> >> in __audit_syscall_exit() (and ..._free()). I don't think this is >> >> really want seccomp wants to be involved in. >> >> >> >> By CONFIG_AUDITSC, you mean CONFIG_AUDITSYSCALL? Without that set, >> >> audit_seccomp is a no-op. >> >> >> >> The reason compat needs to be reported (or rather, arch) is because >> >> just reporting syscall is ambiguous. It either needs arch or compat to >> >> distinguish it. >> > >> > Yes, that is what I mean and you are right. You shouldn't push the >> > syscall in this record either. If !audit_dummy_context() you are >> > already going to get arch, syscall, and a0-a4 in the associated audit >> > record. Please do not duplicate that info. >> >> Ah, in that case, please ignore the patch I just sent. Heh. >> >> > It might make sense to have a separate audit_seccomp() path when >> > audit_dummy_context() which includes arch, syscall, and a0-a4. >> >> Ah! I think I understand what you mean now. If audit_dummy_context(), >> then the syscall, arch, and a0-a4 were not already collected. Gotcha. >> >> How do you envision it looking? I still see it as two distinct events >> (the syscall itself, and the rejection). Would you want those details >> added to the context structure to be reported at ..._exit() time? It >> seems like context->type couldn't be used to see if those fields were >> valid. >> >> Something like: >> >> void __audit_seccomp(unsigned long syscall, long signr) >> { >> struct audit_buffer *ab; >> >> if (!audit_dummy_context()) { >> struct audit_context *context = current->audit_context; >> context->syscall_signr = signr; >> context->syscall_ip = KSTK_EIP(current); >> return; >> } >> >> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND); >> audit_log_abend(ab, "seccomp", signr); >> audit_log_format(ab, " syscall=%ld", syscall); >> audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current)); >> audit_log_end(ab); >> } >> >> And then report syscall_ip and syscall_signr if syscall_signr != 0 in >> the _exit()? I think everything else from audit_log_abend() will end >> up in the _exit() report. >> >> > It is my fault (85e7bac3) that we have syscall at all, but I'm on a new >> > crusade to remove audit record duplication. So I'd happily see a patch >> > in this series that removes that instead of adds to it. >> >> Well, I think the abend reporting is nice; I'd hate to see that >> totally removed. The seccomp case is a bit different, I agree. I could >> see it either way. > > Once again I send you down a bad path. Your original patch was the > best. We should consider including a0-aX in a future version. I was > mistaken in foolishly believing that audit_syscall_entry() was done > before secure_computing(). But if you look, that isn't the case. > Please pretend I never said anything as you had it right the first time. Heh, okay. But now I know more about audit, so that's good. :) -Kees
diff --git a/include/linux/audit.h b/include/linux/audit.h index 9ff7a2c..5aa6cfc 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -463,7 +463,7 @@ extern void audit_putname(const char *name); extern void __audit_inode(const char *name, const struct dentry *dentry); extern void __audit_inode_child(const struct dentry *dentry, const struct inode *parent); -extern void __audit_seccomp(unsigned long syscall); +extern void __audit_seccomp(unsigned long syscall, long signr); extern void __audit_ptrace(struct task_struct *t); static inline int audit_dummy_context(void) @@ -508,10 +508,10 @@ static inline void audit_inode_child(const struct dentry *dentry, } void audit_core_dumps(long signr); -static inline void audit_seccomp(unsigned long syscall) +static inline void audit_seccomp(unsigned long syscall, long signr) { if (unlikely(!audit_dummy_context())) - __audit_seccomp(syscall); + __audit_seccomp(syscall, signr); } static inline void audit_ptrace(struct task_struct *t) @@ -634,7 +634,7 @@ extern int audit_signals; #define audit_inode(n,d) do { (void)(d); } while (0) #define audit_inode_child(i,p) do { ; } while (0) #define audit_core_dumps(i) do { ; } while (0) -#define audit_seccomp(i) do { ; } while (0) +#define audit_seccomp(i,s) do { ; } while (0) #define auditsc_get_stamp(c,t,s) (0) #define audit_get_loginuid(t) (-1) #define audit_get_sessionid(t) (-1) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index af1de0f..74652fe 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -67,6 +67,7 @@ #include <linux/syscalls.h> #include <linux/capability.h> #include <linux/fs_struct.h> +#include <linux/compat.h> #include "audit.h" @@ -2710,13 +2711,17 @@ void audit_core_dumps(long signr) audit_log_end(ab); } -void __audit_seccomp(unsigned long syscall) +void __audit_seccomp(unsigned long syscall, long signr) { struct audit_buffer *ab; ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND); - audit_log_abend(ab, "seccomp", SIGKILL); + audit_log_abend(ab, "seccomp", signr); audit_log_format(ab, " syscall=%ld", syscall); +#ifdef CONFIG_COMPAT + audit_log_format(ab, " compat=%d", is_compat_task()); +#endif + audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current)); audit_log_end(ab); } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 5aabc3c..40af83f 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -57,18 +57,6 @@ struct seccomp_filter { struct sock_filter insns[]; }; -static void seccomp_filter_log_failure(int syscall) -{ - int compat = 0; -#ifdef CONFIG_COMPAT - compat = is_compat_task(); -#endif - pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n", - current->comm, task_pid_nr(current), - (compat ? "compat " : ""), - syscall, KSTK_EIP(current)); -} - /** * get_u32 - returns a u32 offset into data * @data: a unsigned 64 bit value @@ -378,7 +366,6 @@ int __secure_computing_int(int this_syscall) default: break; } - seccomp_filter_log_failure(this_syscall); exit_code = SIGSYS; break; } @@ -390,7 +377,7 @@ int __secure_computing_int(int this_syscall) #ifdef SECCOMP_DEBUG dump_stack(); #endif - audit_seccomp(this_syscall); + audit_seccomp(this_syscall, exit_code); do_exit(exit_code); return -1; /* never reached */ }