diff mbox

[v17,09/15] seccomp: remove duplicated failure logging

Message ID 1333051320-30872-10-git-send-email-wad@chromium.org
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Will Drewry March 29, 2012, 8:01 p.m. UTC 
From: Kees Cook <keescook@chromium.org>

This consolidates the seccomp filter error logging path and adds more
details to the audit log.

Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>

v17: rebase
v16: -
v15: added a return code to the audit_seccomp path by wad@chromium.org
     (suggested by eparis@redhat.com)
v*: original by keescook@chromium.org
---
 include/linux/audit.h |    8 ++++----
 kernel/auditsc.c      |   10 ++++++++--
 kernel/seccomp.c      |   15 +--------------
 3 files changed, 13 insertions(+), 20 deletions(-)

Comments

Andrew Morton April 6, 2012, 9:14 p.m. UTC | #1 
On Thu, 29 Mar 2012 15:01:54 -0500
Will Drewry <wad@chromium.org> wrote:

> From: Kees Cook <keescook@chromium.org>
> 
> This consolidates the seccomp filter error logging path and adds more
> details to the audit log.
> 
> ...
>
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
>
> ...
>
>  #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,c) do { ; } while (0)

Sigh.  Someone please convert all these to C.  That way we get
typechecking and don't need dopey party tricks like that "(void)(d)" to
squish compilation warnings.

> ...
> --- 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,18 @@ void audit_core_dumps(long signr)
>  	audit_log_end(ab);
>  }
>  
> -void __audit_seccomp(unsigned long syscall)
> +void __audit_seccomp(unsigned long syscall, long signr, int code)
>  {
>  	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

We don't need the ifdef for compilation reasons now.

The question is: should we emit the compat= record on
non-compat-capable architectures?  Doing so would be safer - making it
conditional invites people to write x86-only usersapce.


--
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
Will Drewry April 9, 2012, 7:26 p.m. UTC | #2 
On Fri, Apr 6, 2012 at 4:14 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 29 Mar 2012 15:01:54 -0500
> Will Drewry <wad@chromium.org> wrote:
>
>> From: Kees Cook <keescook@chromium.org>
>>
>> This consolidates the seccomp filter error logging path and adds more
>> details to the audit log.
>>
>> ...
>>
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>>
>> ...
>>
>>  #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,c) do { ; } while (0)
>
> Sigh.  Someone please convert all these to C.  That way we get
> typechecking and don't need dopey party tricks like that "(void)(d)" to
> squish compilation warnings.
>
>> ...
>> --- 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,18 @@ void audit_core_dumps(long signr)
>>       audit_log_end(ab);
>>  }
>>
>> -void __audit_seccomp(unsigned long syscall)
>> +void __audit_seccomp(unsigned long syscall, long signr, int code)
>>  {
>>       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
>
> We don't need the ifdef for compilation reasons now.
>
> The question is: should we emit the compat= record on
> non-compat-capable architectures?  Doing so would be safer - making it
> conditional invites people to write x86-only usersapce.

I'd certainly prefer it always being there for exactly that reason.

Kees, Eric, any preferences?  Unless I hear one, I'll just drop the
ifdefs in the next revision.

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
Kees Cook April 9, 2012, 7:32 p.m. UTC | #3 
On Mon, Apr 9, 2012 at 12:26 PM, Will Drewry <wad@chromium.org> wrote:
> On Fri, Apr 6, 2012 at 4:14 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Thu, 29 Mar 2012 15:01:54 -0500
>> Will Drewry <wad@chromium.org> wrote:
>>
>>> From: Kees Cook <keescook@chromium.org>
>>>
>>> This consolidates the seccomp filter error logging path and adds more
>>> details to the audit log.
>>>
>>> ...
>>>
>>> -void __audit_seccomp(unsigned long syscall)
>>> +void __audit_seccomp(unsigned long syscall, long signr, int code)
>>>  {
>>>       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
>>
>> We don't need the ifdef for compilation reasons now.
>>
>> The question is: should we emit the compat= record on
>> non-compat-capable architectures?  Doing so would be safer - making it
>> conditional invites people to write x86-only usersapce.
>
> I'd certainly prefer it always being there for exactly that reason.
>
> Kees, Eric, any preferences?  Unless I hear one, I'll just drop the
> ifdefs in the next revision.

Yeah, I'd prefer the ifdefs dropped too.

-Kees
Eric Paris April 9, 2012, 7:33 p.m. UTC | #4 
On Mon, 2012-04-09 at 14:26 -0500, Will Drewry wrote:
> On Fri, Apr 6, 2012 at 4:14 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Thu, 29 Mar 2012 15:01:54 -0500
> > Will Drewry <wad@chromium.org> wrote:

> >> -void __audit_seccomp(unsigned long syscall)
> >> +void __audit_seccomp(unsigned long syscall, long signr, int code)
> >>  {
> >>       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
> >
> > We don't need the ifdef for compilation reasons now.
> >
> > The question is: should we emit the compat= record on
> > non-compat-capable architectures?  Doing so would be safer - making it
> > conditional invites people to write x86-only usersapce.
> 
> I'd certainly prefer it always being there for exactly that reason.
> 
> Kees, Eric, any preferences?  Unless I hear one, I'll just drop the
> ifdefs in the next revision.

I'd just leave it in unconditionally.  The audit parse libraries would
handle it just fine, but that doesn't mean everyone uses that tool to
parse the text.

-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
Kees Cook April 9, 2012, 7:39 p.m. UTC | #5 
On Mon, Apr 9, 2012 at 12:33 PM, Eric Paris <eparis@redhat.com> wrote:
> On Mon, 2012-04-09 at 14:26 -0500, Will Drewry wrote:
>> On Fri, Apr 6, 2012 at 4:14 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > On Thu, 29 Mar 2012 15:01:54 -0500
>> > Will Drewry <wad@chromium.org> wrote:
>
>> >> -void __audit_seccomp(unsigned long syscall)
>> >> +void __audit_seccomp(unsigned long syscall, long signr, int code)
>> >>  {
>> >>       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
>> >
>> > We don't need the ifdef for compilation reasons now.
>> >
>> > The question is: should we emit the compat= record on
>> > non-compat-capable architectures?  Doing so would be safer - making it
>> > conditional invites people to write x86-only usersapce.
>>
>> I'd certainly prefer it always being there for exactly that reason.
>>
>> Kees, Eric, any preferences?  Unless I hear one, I'll just drop the
>> ifdefs in the next revision.
>
> I'd just leave it in unconditionally.  The audit parse libraries would
> handle it just fine, but that doesn't mean everyone uses that tool to
> parse the text.

Related to this, can we get this patch into a tree as well?
https://lkml.org/lkml/2012/3/23/332

Thanks,

-Kees
diff mbox

Patch

diff --git a/include/linux/audit.h b/include/linux/audit.h
index ed3ef19..22f292a 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, int code);
 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, int code)
 {
 	if (unlikely(!audit_dummy_context()))
-		__audit_seccomp(syscall);
+		__audit_seccomp(syscall, signr, code);
 }
 
 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,c) 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..10dc528 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,18 @@  void audit_core_dumps(long signr)
 	audit_log_end(ab);
 }
 
-void __audit_seccomp(unsigned long syscall)
+void __audit_seccomp(unsigned long syscall, long signr, int code)
 {
 	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_format(ab, " code=0x%x", code);
 	audit_log_end(ab);
 }
 
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 5fb2d57..85cbe37 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -60,18 +60,6 @@  struct seccomp_filter {
 /* Limit any path through the tree to 256KB worth of instructions. */
 #define MAX_INSNS_PER_PATH ((1 << 18) / sizeof(struct sock_filter))
 
-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
@@ -376,7 +364,6 @@  void __secure_computing(int this_syscall)
 	case SECCOMP_MODE_FILTER:
 		if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW)
 			return;
-		seccomp_filter_log_failure(this_syscall);
 		exit_sig = SIGSYS;
 		break;
 #endif
@@ -387,7 +374,7 @@  void __secure_computing(int this_syscall)
 #ifdef SECCOMP_DEBUG
 	dump_stack();
 #endif
-	audit_seccomp(this_syscall);
+	audit_seccomp(this_syscall, exit_code, SECCOMP_RET_KILL);
 	do_exit(exit_sig);
 }