linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
@ 2019-04-09  3:52 Richard Guy Briggs
  2019-04-09  6:01 ` Steve Grubb
  2019-04-18 14:59 ` Paul Moore
  0 siblings, 2 replies; 12+ messages in thread
From: Richard Guy Briggs @ 2019-04-09  3:52 UTC (permalink / raw)
  To: LKML, Linux-Audit Mailing List
  Cc: Paul Moore, sgrubb, omosnace, eparis, ebiederm, oleg, Richard Guy Briggs

When a process signals the audit daemon (shutdown, rotate, resume,
reconfig) but syscall auditing is not enabled, we still want to know the
identity of the process sending the signal to the audit daemon.

Move audit_signal_info() out of syscall auditing to general auditing but
create a new function audit_signal_info_syscall() to take care of the
syscall dependent parts for when syscall auditing is enabled.

Please see the github kernel audit issue
https://github.com/linux-audit/audit-kernel/issues/111

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h |  6 ++++++
 kernel/audit.c        | 27 +++++++++++++++++++++++++++
 kernel/audit.h        |  4 ++--
 kernel/auditsc.c      | 19 +++----------------
 kernel/signal.c       |  2 +-
 5 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 1e69d9fe16da..4a22fc3f824f 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -173,6 +173,9 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 }
 
 extern u32 audit_enabled;
+
+extern int audit_signal_info(int sig, struct task_struct *t);
+
 #else /* CONFIG_AUDIT */
 static inline __printf(4, 5)
 void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
@@ -226,6 +229,9 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 }
 
 #define audit_enabled AUDIT_OFF
+
+#define audit_signal_info(s, t) AUDIT_OFF
+
 #endif /* CONFIG_AUDIT */
 
 #ifdef CONFIG_AUDIT_COMPAT_GENERIC
diff --git a/kernel/audit.c b/kernel/audit.c
index b96bf69183f4..67399ff72d43 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2274,6 +2274,33 @@ int audit_set_loginuid(kuid_t loginuid)
 }
 
 /**
+ * audit_signal_info - record signal info for shutting down audit subsystem
+ * @sig: signal value
+ * @t: task being signaled
+ *
+ * If the audit subsystem is being terminated, record the task (pid)
+ * and uid that is doing that.
+ */
+int audit_signal_info(int sig, struct task_struct *t)
+{
+	kuid_t uid = current_uid(), auid;
+
+	if (auditd_test_task(t) &&
+	    (sig == SIGTERM || sig == SIGHUP ||
+	     sig == SIGUSR1 || sig == SIGUSR2)) {
+		audit_sig_pid = task_tgid_nr(current);
+		auid = audit_get_loginuid(current);
+		if (uid_valid(auid))
+			audit_sig_uid = auid;
+		else
+			audit_sig_uid = uid;
+		security_task_getsecid(current, &audit_sig_sid);
+	}
+
+	return audit_signal_info_syscall(t);
+}
+
+/**
  * audit_log_end - end one audit record
  * @ab: the audit_buffer
  *
diff --git a/kernel/audit.h b/kernel/audit.h
index 958d5b8fc1b3..18a8ae812e9f 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk *chunk,
 extern void audit_put_tree(struct audit_tree *tree);
 extern void audit_kill_trees(struct audit_context *context);
 
-extern int audit_signal_info(int sig, struct task_struct *t);
+extern int audit_signal_info_syscall(struct task_struct *t);
 extern void audit_filter_inodes(struct task_struct *tsk,
 				struct audit_context *ctx);
 extern struct list_head *audit_killed_trees(void);
@@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
 #define audit_tree_path(rule) ""	/* never called */
 #define audit_kill_trees(context) BUG()
 
-#define audit_signal_info(s, t) AUDIT_DISABLED
+#define audit_signal_info_syscall(t) AUDIT_OFF
 #define audit_filter_inodes(t, c) AUDIT_DISABLED
 #endif /* CONFIG_AUDITSYSCALL */
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 98a98e6dca05..dbd43d84c347 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2370,30 +2370,17 @@ void __audit_ptrace(struct task_struct *t)
 }
 
 /**
- * audit_signal_info - record signal info for shutting down audit subsystem
- * @sig: signal value
+ * audit_signal_info_syscall - record signal info for syscalls
  * @t: task being signaled
  *
  * If the audit subsystem is being terminated, record the task (pid)
  * and uid that is doing that.
  */
-int audit_signal_info(int sig, struct task_struct *t)
+int audit_signal_info_syscall(struct task_struct *t)
 {
 	struct audit_aux_data_pids *axp;
 	struct audit_context *ctx = audit_context();
-	kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
-
-	if (auditd_test_task(t) &&
-	    (sig == SIGTERM || sig == SIGHUP ||
-	     sig == SIGUSR1 || sig == SIGUSR2)) {
-		audit_sig_pid = task_tgid_nr(current);
-		auid = audit_get_loginuid(current);
-		if (uid_valid(auid))
-			audit_sig_uid = auid;
-		else
-			audit_sig_uid = uid;
-		security_task_getsecid(current, &audit_sig_sid);
-	}
+	kuid_t t_uid = task_uid(t);
 
 	if (!audit_signals || audit_dummy_context())
 		return 0;
diff --git a/kernel/signal.c b/kernel/signal.c
index b7953934aa99..73db5dfa797d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -43,6 +43,7 @@
 #include <linux/compiler.h>
 #include <linux/posix-timers.h>
 #include <linux/livepatch.h>
+#include <linux/audit.h>	/* audit_signal_info() */
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/signal.h>
@@ -52,7 +53,6 @@
 #include <asm/unistd.h>
 #include <asm/siginfo.h>
 #include <asm/cacheflush.h>
-#include "audit.h"	/* audit_signal_info() */
 
 /*
  * SLAB caches for signal bits.
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-09  3:52 [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall Richard Guy Briggs
@ 2019-04-09  6:01 ` Steve Grubb
  2019-04-09 14:02   ` Richard Guy Briggs
  2019-04-18 14:59 ` Paul Moore
  1 sibling, 1 reply; 12+ messages in thread
From: Steve Grubb @ 2019-04-09  6:01 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: LKML, Linux-Audit Mailing List, Paul Moore, omosnace, eparis,
	ebiederm, oleg

On Mon,  8 Apr 2019 23:52:29 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:

> When a process signals the audit daemon (shutdown, rotate, resume,
> reconfig) but syscall auditing is not enabled, we still want to know
> the identity of the process sending the signal to the audit daemon.

Why? If syscall auditing is disabled, then there is no requirement to
provide anything. What is the real problem that you are seeing?

Thanks,
-Steve

> Move audit_signal_info() out of syscall auditing to general auditing
> but create a new function audit_signal_info_syscall() to take care of
> the syscall dependent parts for when syscall auditing is enabled.
> 
> Please see the github kernel audit issue
> https://github.com/linux-audit/audit-kernel/issues/111
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |  6 ++++++
>  kernel/audit.c        | 27 +++++++++++++++++++++++++++
>  kernel/audit.h        |  4 ++--
>  kernel/auditsc.c      | 19 +++----------------
>  kernel/signal.c       |  2 +-
>  5 files changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 1e69d9fe16da..4a22fc3f824f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -173,6 +173,9 @@ static inline unsigned int
> audit_get_sessionid(struct task_struct *tsk) }
>  
>  extern u32 audit_enabled;
> +
> +extern int audit_signal_info(int sig, struct task_struct *t);
> +
>  #else /* CONFIG_AUDIT */
>  static inline __printf(4, 5)
>  void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
> @@ -226,6 +229,9 @@ static inline unsigned int
> audit_get_sessionid(struct task_struct *tsk) }
>  
>  #define audit_enabled AUDIT_OFF
> +
> +#define audit_signal_info(s, t) AUDIT_OFF
> +
>  #endif /* CONFIG_AUDIT */
>  
>  #ifdef CONFIG_AUDIT_COMPAT_GENERIC
> diff --git a/kernel/audit.c b/kernel/audit.c
> index b96bf69183f4..67399ff72d43 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2274,6 +2274,33 @@ int audit_set_loginuid(kuid_t loginuid)
>  }
>  
>  /**
> + * audit_signal_info - record signal info for shutting down audit
> subsystem
> + * @sig: signal value
> + * @t: task being signaled
> + *
> + * If the audit subsystem is being terminated, record the task (pid)
> + * and uid that is doing that.
> + */
> +int audit_signal_info(int sig, struct task_struct *t)
> +{
> +	kuid_t uid = current_uid(), auid;
> +
> +	if (auditd_test_task(t) &&
> +	    (sig == SIGTERM || sig == SIGHUP ||
> +	     sig == SIGUSR1 || sig == SIGUSR2)) {
> +		audit_sig_pid = task_tgid_nr(current);
> +		auid = audit_get_loginuid(current);
> +		if (uid_valid(auid))
> +			audit_sig_uid = auid;
> +		else
> +			audit_sig_uid = uid;
> +		security_task_getsecid(current, &audit_sig_sid);
> +	}
> +
> +	return audit_signal_info_syscall(t);
> +}
> +
> +/**
>   * audit_log_end - end one audit record
>   * @ab: the audit_buffer
>   *
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 958d5b8fc1b3..18a8ae812e9f 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk
> *chunk, extern void audit_put_tree(struct audit_tree *tree);
>  extern void audit_kill_trees(struct audit_context *context);
>  
> -extern int audit_signal_info(int sig, struct task_struct *t);
> +extern int audit_signal_info_syscall(struct task_struct *t);
>  extern void audit_filter_inodes(struct task_struct *tsk,
>  				struct audit_context *ctx);
>  extern struct list_head *audit_killed_trees(void);
> @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct
> task_struct *tsk, #define audit_tree_path(rule) ""	/* never
> called */ #define audit_kill_trees(context) BUG()
>  
> -#define audit_signal_info(s, t) AUDIT_DISABLED
> +#define audit_signal_info_syscall(t) AUDIT_OFF
>  #define audit_filter_inodes(t, c) AUDIT_DISABLED
>  #endif /* CONFIG_AUDITSYSCALL */
>  
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 98a98e6dca05..dbd43d84c347 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2370,30 +2370,17 @@ void __audit_ptrace(struct task_struct *t)
>  }
>  
>  /**
> - * audit_signal_info - record signal info for shutting down audit
> subsystem
> - * @sig: signal value
> + * audit_signal_info_syscall - record signal info for syscalls
>   * @t: task being signaled
>   *
>   * If the audit subsystem is being terminated, record the task (pid)
>   * and uid that is doing that.
>   */
> -int audit_signal_info(int sig, struct task_struct *t)
> +int audit_signal_info_syscall(struct task_struct *t)
>  {
>  	struct audit_aux_data_pids *axp;
>  	struct audit_context *ctx = audit_context();
> -	kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
> -
> -	if (auditd_test_task(t) &&
> -	    (sig == SIGTERM || sig == SIGHUP ||
> -	     sig == SIGUSR1 || sig == SIGUSR2)) {
> -		audit_sig_pid = task_tgid_nr(current);
> -		auid = audit_get_loginuid(current);
> -		if (uid_valid(auid))
> -			audit_sig_uid = auid;
> -		else
> -			audit_sig_uid = uid;
> -		security_task_getsecid(current, &audit_sig_sid);
> -	}
> +	kuid_t t_uid = task_uid(t);
>  
>  	if (!audit_signals || audit_dummy_context())
>  		return 0;
> diff --git a/kernel/signal.c b/kernel/signal.c
> index b7953934aa99..73db5dfa797d 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -43,6 +43,7 @@
>  #include <linux/compiler.h>
>  #include <linux/posix-timers.h>
>  #include <linux/livepatch.h>
> +#include <linux/audit.h>	/* audit_signal_info() */
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/signal.h>
> @@ -52,7 +53,6 @@
>  #include <asm/unistd.h>
>  #include <asm/siginfo.h>
>  #include <asm/cacheflush.h>
> -#include "audit.h"	/* audit_signal_info() */
>  
>  /*
>   * SLAB caches for signal bits.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-09  6:01 ` Steve Grubb
@ 2019-04-09 14:02   ` Richard Guy Briggs
  2019-04-09 15:37     ` Steve Grubb
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Guy Briggs @ 2019-04-09 14:02 UTC (permalink / raw)
  To: Steve Grubb
  Cc: LKML, Linux-Audit Mailing List, Paul Moore, omosnace, eparis,
	ebiederm, oleg

On 2019-04-09 08:01, Steve Grubb wrote:
> On Mon,  8 Apr 2019 23:52:29 -0400 Richard Guy Briggs <rgb@redhat.com> wrote:
> > When a process signals the audit daemon (shutdown, rotate, resume,
> > reconfig) but syscall auditing is not enabled, we still want to know
> > the identity of the process sending the signal to the audit daemon.
> 
> Why? If syscall auditing is disabled, then there is no requirement to
> provide anything. What is the real problem that you are seeing?

Shutdown messages with -1 in them rather than the real values.

> -Steve
> 
> > Move audit_signal_info() out of syscall auditing to general auditing
> > but create a new function audit_signal_info_syscall() to take care of
> > the syscall dependent parts for when syscall auditing is enabled.
> > 
> > Please see the github kernel audit issue
> > https://github.com/linux-audit/audit-kernel/issues/111
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h |  6 ++++++
> >  kernel/audit.c        | 27 +++++++++++++++++++++++++++
> >  kernel/audit.h        |  4 ++--
> >  kernel/auditsc.c      | 19 +++----------------
> >  kernel/signal.c       |  2 +-
> >  5 files changed, 39 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 1e69d9fe16da..4a22fc3f824f 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -173,6 +173,9 @@ static inline unsigned int
> > audit_get_sessionid(struct task_struct *tsk) }
> >  
> >  extern u32 audit_enabled;
> > +
> > +extern int audit_signal_info(int sig, struct task_struct *t);
> > +
> >  #else /* CONFIG_AUDIT */
> >  static inline __printf(4, 5)
> >  void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
> > @@ -226,6 +229,9 @@ static inline unsigned int
> > audit_get_sessionid(struct task_struct *tsk) }
> >  
> >  #define audit_enabled AUDIT_OFF
> > +
> > +#define audit_signal_info(s, t) AUDIT_OFF
> > +
> >  #endif /* CONFIG_AUDIT */
> >  
> >  #ifdef CONFIG_AUDIT_COMPAT_GENERIC
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index b96bf69183f4..67399ff72d43 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2274,6 +2274,33 @@ int audit_set_loginuid(kuid_t loginuid)
> >  }
> >  
> >  /**
> > + * audit_signal_info - record signal info for shutting down audit
> > subsystem
> > + * @sig: signal value
> > + * @t: task being signaled
> > + *
> > + * If the audit subsystem is being terminated, record the task (pid)
> > + * and uid that is doing that.
> > + */
> > +int audit_signal_info(int sig, struct task_struct *t)
> > +{
> > +	kuid_t uid = current_uid(), auid;
> > +
> > +	if (auditd_test_task(t) &&
> > +	    (sig == SIGTERM || sig == SIGHUP ||
> > +	     sig == SIGUSR1 || sig == SIGUSR2)) {
> > +		audit_sig_pid = task_tgid_nr(current);
> > +		auid = audit_get_loginuid(current);
> > +		if (uid_valid(auid))
> > +			audit_sig_uid = auid;
> > +		else
> > +			audit_sig_uid = uid;
> > +		security_task_getsecid(current, &audit_sig_sid);
> > +	}
> > +
> > +	return audit_signal_info_syscall(t);
> > +}
> > +
> > +/**
> >   * audit_log_end - end one audit record
> >   * @ab: the audit_buffer
> >   *
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 958d5b8fc1b3..18a8ae812e9f 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk
> > *chunk, extern void audit_put_tree(struct audit_tree *tree);
> >  extern void audit_kill_trees(struct audit_context *context);
> >  
> > -extern int audit_signal_info(int sig, struct task_struct *t);
> > +extern int audit_signal_info_syscall(struct task_struct *t);
> >  extern void audit_filter_inodes(struct task_struct *tsk,
> >  				struct audit_context *ctx);
> >  extern struct list_head *audit_killed_trees(void);
> > @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct
> > task_struct *tsk, #define audit_tree_path(rule) ""	/* never
> > called */ #define audit_kill_trees(context) BUG()
> >  
> > -#define audit_signal_info(s, t) AUDIT_DISABLED
> > +#define audit_signal_info_syscall(t) AUDIT_OFF
> >  #define audit_filter_inodes(t, c) AUDIT_DISABLED
> >  #endif /* CONFIG_AUDITSYSCALL */
> >  
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 98a98e6dca05..dbd43d84c347 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2370,30 +2370,17 @@ void __audit_ptrace(struct task_struct *t)
> >  }
> >  
> >  /**
> > - * audit_signal_info - record signal info for shutting down audit
> > subsystem
> > - * @sig: signal value
> > + * audit_signal_info_syscall - record signal info for syscalls
> >   * @t: task being signaled
> >   *
> >   * If the audit subsystem is being terminated, record the task (pid)
> >   * and uid that is doing that.
> >   */
> > -int audit_signal_info(int sig, struct task_struct *t)
> > +int audit_signal_info_syscall(struct task_struct *t)
> >  {
> >  	struct audit_aux_data_pids *axp;
> >  	struct audit_context *ctx = audit_context();
> > -	kuid_t uid = current_uid(), auid, t_uid = task_uid(t);
> > -
> > -	if (auditd_test_task(t) &&
> > -	    (sig == SIGTERM || sig == SIGHUP ||
> > -	     sig == SIGUSR1 || sig == SIGUSR2)) {
> > -		audit_sig_pid = task_tgid_nr(current);
> > -		auid = audit_get_loginuid(current);
> > -		if (uid_valid(auid))
> > -			audit_sig_uid = auid;
> > -		else
> > -			audit_sig_uid = uid;
> > -		security_task_getsecid(current, &audit_sig_sid);
> > -	}
> > +	kuid_t t_uid = task_uid(t);
> >  
> >  	if (!audit_signals || audit_dummy_context())
> >  		return 0;
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index b7953934aa99..73db5dfa797d 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -43,6 +43,7 @@
> >  #include <linux/compiler.h>
> >  #include <linux/posix-timers.h>
> >  #include <linux/livepatch.h>
> > +#include <linux/audit.h>	/* audit_signal_info() */
> >  
> >  #define CREATE_TRACE_POINTS
> >  #include <trace/events/signal.h>
> > @@ -52,7 +53,6 @@
> >  #include <asm/unistd.h>
> >  #include <asm/siginfo.h>
> >  #include <asm/cacheflush.h>
> > -#include "audit.h"	/* audit_signal_info() */
> >  
> >  /*
> >   * SLAB caches for signal bits.
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-09 14:02   ` Richard Guy Briggs
@ 2019-04-09 15:37     ` Steve Grubb
  2019-04-09 15:57       ` Richard Guy Briggs
  0 siblings, 1 reply; 12+ messages in thread
From: Steve Grubb @ 2019-04-09 15:37 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: LKML, Linux-Audit Mailing List, Paul Moore, omosnace, eparis,
	ebiederm, oleg

On Tue, 9 Apr 2019 10:02:59 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:

> On 2019-04-09 08:01, Steve Grubb wrote:
> > On Mon,  8 Apr 2019 23:52:29 -0400 Richard Guy Briggs
> > <rgb@redhat.com> wrote:  
> > > When a process signals the audit daemon (shutdown, rotate, resume,
> > > reconfig) but syscall auditing is not enabled, we still want to
> > > know the identity of the process sending the signal to the audit
> > > daemon.  
> > 
> > Why? If syscall auditing is disabled, then there is no requirement
> > to provide anything. What is the real problem that you are seeing?  
> 
> Shutdown messages with -1 in them rather than the real values.

OK. We can fix that by patching auditd to see if auditing is enabled
before requesting signal info. If auditing is disabled, the proper
action is for the kernel to ignore any audit userspace messages except
the configuration commands.

-Steve

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-09 15:37     ` Steve Grubb
@ 2019-04-09 15:57       ` Richard Guy Briggs
  2019-04-10  0:25         ` Eric W. Biederman
  2019-04-11 12:22         ` Steve Grubb
  0 siblings, 2 replies; 12+ messages in thread
From: Richard Guy Briggs @ 2019-04-09 15:57 UTC (permalink / raw)
  To: Steve Grubb
  Cc: LKML, Linux-Audit Mailing List, Paul Moore, omosnace, eparis,
	ebiederm, oleg

On 2019-04-09 17:37, Steve Grubb wrote:
> On Tue, 9 Apr 2019 10:02:59 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
> 
> > On 2019-04-09 08:01, Steve Grubb wrote:
> > > On Mon,  8 Apr 2019 23:52:29 -0400 Richard Guy Briggs
> > > <rgb@redhat.com> wrote:  
> > > > When a process signals the audit daemon (shutdown, rotate, resume,
> > > > reconfig) but syscall auditing is not enabled, we still want to
> > > > know the identity of the process sending the signal to the audit
> > > > daemon.  
> > > 
> > > Why? If syscall auditing is disabled, then there is no requirement
> > > to provide anything. What is the real problem that you are seeing?  
> > 
> > Shutdown messages with -1 in them rather than the real values.
> 
> OK. We can fix that by patching auditd to see if auditing is enabled
> before requesting signal info. If auditing is disabled, the proper
> action is for the kernel to ignore any audit userspace messages except
> the configuration commands.

If auditing is disabled in the kernel, none of this is trackable.  It is
for those as yet unsupported arches that can run audit enabled but
without auditsyscall support.

Here's a current sample with CONFIG_AUDIT and ~CONFIG_AUDITSYSCALL
configured, note "auid=" and "pid=":

	killall -HUP auditd
	type=DAEMON_CONFIG msg=audit(2019-04-09 11:37:04.508:3266) op=reconfigure state=changed auid=unset pid=-1 subj=? res=success

	killall -TERM auditd
	type=DAEMON_END msg=audit(2019-04-09 11:51:50.441:5709) : op=terminate auid=unset pid=-1 subj=? res=success 
	
and the same with the patch applied:

	killall -HUP auditd
	type=DAEMON_CONFIG msg=audit(2019-04-09 11:42:40.851:8924) op=reconfigure state=changed auid=root pid=652 subj=? res=success

	killall -TERM auditd
	type=DAEMON_END msg=audit(2019-04-09 11:51:50.441:5709) : op=terminate auid=root pid=652 subj=? res=success 

The USR1 "rotate" and USR2 "resume" log messages need to be fixed in
userspace.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-09 15:57       ` Richard Guy Briggs
@ 2019-04-10  0:25         ` Eric W. Biederman
  2019-04-10 16:54           ` Richard Guy Briggs
  2019-04-11 12:22         ` Steve Grubb
  1 sibling, 1 reply; 12+ messages in thread
From: Eric W. Biederman @ 2019-04-10  0:25 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Steve Grubb, LKML, Linux-Audit Mailing List, Paul Moore,
	omosnace, eparis, oleg


Minor nit about the description of this patch (as I presume it will need
to respun).

You are talking about audit signal information.  You are not talking
about struct siginfo (aka siginfo_t).  The structure siginfo_t is part
of posix and userspace ABI and is part of the stack frame for a
delivered signal.

The summary had me thinking you were messing with when siginfo is
collected and delivered to userspace.

Richard Guy Briggs <rgb@redhat.com> writes:

> On 2019-04-09 17:37, Steve Grubb wrote:
>> On Tue, 9 Apr 2019 10:02:59 -0400
>> Richard Guy Briggs <rgb@redhat.com> wrote:
>> 
>> > On 2019-04-09 08:01, Steve Grubb wrote:
>> > > On Mon,  8 Apr 2019 23:52:29 -0400 Richard Guy Briggs
>> > > <rgb@redhat.com> wrote:  
>> > > > When a process signals the audit daemon (shutdown, rotate, resume,
>> > > > reconfig) but syscall auditing is not enabled, we still want to
>> > > > know the identity of the process sending the signal to the audit
>> > > > daemon.  
>> > > 
>> > > Why? If syscall auditing is disabled, then there is no requirement
>> > > to provide anything. What is the real problem that you are seeing?  
>> > 
>> > Shutdown messages with -1 in them rather than the real values.
>> 
>> OK. We can fix that by patching auditd to see if auditing is enabled
>> before requesting signal info. If auditing is disabled, the proper
>> action is for the kernel to ignore any audit userspace messages except
>> the configuration commands.
>
> If auditing is disabled in the kernel, none of this is trackable.  It is
> for those as yet unsupported arches that can run audit enabled but
> without auditsyscall support.
>
> Here's a current sample with CONFIG_AUDIT and ~CONFIG_AUDITSYSCALL
> configured, note "auid=" and "pid=":
>
> 	killall -HUP auditd
> 	type=DAEMON_CONFIG msg=audit(2019-04-09 11:37:04.508:3266) op=reconfigure state=changed auid=unset pid=-1 subj=? res=success
>
> 	killall -TERM auditd
> 	type=DAEMON_END msg=audit(2019-04-09 11:51:50.441:5709) : op=terminate auid=unset pid=-1 subj=? res=success 
> 	
> and the same with the patch applied:
>
> 	killall -HUP auditd
> 	type=DAEMON_CONFIG msg=audit(2019-04-09 11:42:40.851:8924) op=reconfigure state=changed auid=root pid=652 subj=? res=success
>
> 	killall -TERM auditd
> 	type=DAEMON_END msg=audit(2019-04-09 11:51:50.441:5709) : op=terminate auid=root pid=652 subj=? res=success 
>
> The USR1 "rotate" and USR2 "resume" log messages need to be fixed in
> userspace.

Hmm.  You mention -1 as beeing a problem.  You don't say if auid is a
concern.

It looks like all you care about is the sending processes pid.  That
information is saved in the sending processes siginfo.  As such you may
be able to remove some of the magic from the code by looking at the
siginfo of the signal.

Why it appears the kernel is logging when userspace receives a signal is
beyond me so I don't know using siginfo makes sense.  I just figure I
will toss it out there in case it shakes some ideas loose.

Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-10  0:25         ` Eric W. Biederman
@ 2019-04-10 16:54           ` Richard Guy Briggs
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Guy Briggs @ 2019-04-10 16:54 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Steve Grubb, LKML, Linux-Audit Mailing List, Paul Moore,
	omosnace, eparis, oleg

On 2019-04-09 19:25, Eric W. Biederman wrote:
> Minor nit about the description of this patch (as I presume it will need
> to respun).
> 
> You are talking about audit signal information.  You are not talking
> about struct siginfo (aka siginfo_t).  The structure siginfo_t is part
> of posix and userspace ABI and is part of the stack frame for a
> delivered signal.
> 
> The summary had me thinking you were messing with when siginfo is
> collected and delivered to userspace.

Got it.  I'll switch it to at least sig_info if not something even a bit
more descriptive and less confusing.

> Richard Guy Briggs <rgb@redhat.com> writes:
> > On 2019-04-09 17:37, Steve Grubb wrote:
> >> On Tue, 9 Apr 2019 10:02:59 -0400
> >> Richard Guy Briggs <rgb@redhat.com> wrote:
> >> 
> >> > On 2019-04-09 08:01, Steve Grubb wrote:
> >> > > On Mon,  8 Apr 2019 23:52:29 -0400 Richard Guy Briggs
> >> > > <rgb@redhat.com> wrote:  
> >> > > > When a process signals the audit daemon (shutdown, rotate, resume,
> >> > > > reconfig) but syscall auditing is not enabled, we still want to
> >> > > > know the identity of the process sending the signal to the audit
> >> > > > daemon.  
> >> > > 
> >> > > Why? If syscall auditing is disabled, then there is no requirement
> >> > > to provide anything. What is the real problem that you are seeing?  
> >> > 
> >> > Shutdown messages with -1 in them rather than the real values.
> >> 
> >> OK. We can fix that by patching auditd to see if auditing is enabled
> >> before requesting signal info. If auditing is disabled, the proper
> >> action is for the kernel to ignore any audit userspace messages except
> >> the configuration commands.
> >
> > If auditing is disabled in the kernel, none of this is trackable.  It is
> > for those as yet unsupported arches that can run audit enabled but
> > without auditsyscall support.
> >
> > Here's a current sample with CONFIG_AUDIT and ~CONFIG_AUDITSYSCALL
> > configured, note "auid=" and "pid=":
> >
> > 	killall -HUP auditd
> > 	type=DAEMON_CONFIG msg=audit(2019-04-09 11:37:04.508:3266) op=reconfigure state=changed auid=unset pid=-1 subj=? res=success
> >
> > 	killall -TERM auditd
> > 	type=DAEMON_END msg=audit(2019-04-09 11:51:50.441:5709) : op=terminate auid=unset pid=-1 subj=? res=success 
> > 	
> > and the same with the patch applied:
> >
> > 	killall -HUP auditd
> > 	type=DAEMON_CONFIG msg=audit(2019-04-09 11:42:40.851:8924) op=reconfigure state=changed auid=root pid=652 subj=? res=success
> >
> > 	killall -TERM auditd
> > 	type=DAEMON_END msg=audit(2019-04-09 11:51:50.441:5709) : op=terminate auid=root pid=652 subj=? res=success 
> >
> > The USR1 "rotate" and USR2 "resume" log messages need to be fixed in
> > userspace.
> 
> Hmm.  You mention -1 as beeing a problem.  You don't say if auid is a
> concern.

Ok, -1 can be a real value if it isn't actually set, but in this case,
there is information available that isn't being used.

> It looks like all you care about is the sending processes pid.  That
> information is saved in the sending processes siginfo.  As such you may
> be able to remove some of the magic from the code by looking at the
> siginfo of the signal.

We need the sending process' pid, auid and ses, as well as its security
context label and the reason I noticed all this is that soon we'll also
want the audit container identifier as well.

So some of this information is available in siginfo, but I don't know if
all of it is.

> Why it appears the kernel is logging when userspace receives a signal is
> beyond me so I don't know using siginfo makes sense.  I just figure I
> will toss it out there in case it shakes some ideas loose.

That is worth checking to see if it is all available.

Thanks, Eric.

> Eric

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-09 15:57       ` Richard Guy Briggs
  2019-04-10  0:25         ` Eric W. Biederman
@ 2019-04-11 12:22         ` Steve Grubb
  1 sibling, 0 replies; 12+ messages in thread
From: Steve Grubb @ 2019-04-11 12:22 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: LKML, Linux-Audit Mailing List, Paul Moore, omosnace, eparis,
	ebiederm, oleg

On Tue, 9 Apr 2019 11:57:28 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:

> On 2019-04-09 17:37, Steve Grubb wrote:
> > On Tue, 9 Apr 2019 10:02:59 -0400
> > Richard Guy Briggs <rgb@redhat.com> wrote:
> >   
> > > On 2019-04-09 08:01, Steve Grubb wrote:  
> > > > On Mon,  8 Apr 2019 23:52:29 -0400 Richard Guy Briggs
> > > > <rgb@redhat.com> wrote:    
> > > > > When a process signals the audit daemon (shutdown, rotate,
> > > > > resume, reconfig) but syscall auditing is not enabled, we
> > > > > still want to know the identity of the process sending the
> > > > > signal to the audit daemon.    
> > > > 
> > > > Why? If syscall auditing is disabled, then there is no
> > > > requirement to provide anything. What is the real problem that
> > > > you are seeing?    
> > > 
> > > Shutdown messages with -1 in them rather than the real values.  
> > 
> > OK. We can fix that by patching auditd to see if auditing is enabled
> > before requesting signal info. If auditing is disabled, the proper
> > action is for the kernel to ignore any audit userspace messages
> > except the configuration commands.  
> 
> If auditing is disabled in the kernel, none of this is trackable.  It
> is for those as yet unsupported arches that can run audit enabled but
> without auditsyscall support.

Ok. I suppose this is useful for this use case. No further objections.

-Steve

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-09  3:52 [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall Richard Guy Briggs
  2019-04-09  6:01 ` Steve Grubb
@ 2019-04-18 14:59 ` Paul Moore
  2019-04-18 15:16   ` Richard Guy Briggs
  1 sibling, 1 reply; 12+ messages in thread
From: Paul Moore @ 2019-04-18 14:59 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: LKML, Linux-Audit Mailing List, sgrubb, omosnace, Eric Paris,
	ebiederm, oleg

On Mon, Apr 8, 2019 at 11:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> When a process signals the audit daemon (shutdown, rotate, resume,
> reconfig) but syscall auditing is not enabled, we still want to know the
> identity of the process sending the signal to the audit daemon.
>
> Move audit_signal_info() out of syscall auditing to general auditing but
> create a new function audit_signal_info_syscall() to take care of the
> syscall dependent parts for when syscall auditing is enabled.
>
> Please see the github kernel audit issue
> https://github.com/linux-audit/audit-kernel/issues/111
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |  6 ++++++
>  kernel/audit.c        | 27 +++++++++++++++++++++++++++
>  kernel/audit.h        |  4 ++--
>  kernel/auditsc.c      | 19 +++----------------
>  kernel/signal.c       |  2 +-
>  5 files changed, 39 insertions(+), 19 deletions(-)

...

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 1e69d9fe16da..4a22fc3f824f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -226,6 +229,9 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>  }
>
>  #define audit_enabled AUDIT_OFF
> +
> +#define audit_signal_info(s, t) AUDIT_OFF
> +

Should this be AUDIT_DISABLED to preserve the current value/behavior?
Technically they should both have a value of zero right now, but since
the AUDIT_DISABLED value isn't explicit it seems safer to go with
AUDIT_DISABLED.

> diff --git a/kernel/audit.h b/kernel/audit.h
> index 958d5b8fc1b3..18a8ae812e9f 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk *chunk,
>  extern void audit_put_tree(struct audit_tree *tree);
>  extern void audit_kill_trees(struct audit_context *context);
>
> -extern int audit_signal_info(int sig, struct task_struct *t);
> +extern int audit_signal_info_syscall(struct task_struct *t);
>  extern void audit_filter_inodes(struct task_struct *tsk,
>                                 struct audit_context *ctx);
>  extern struct list_head *audit_killed_trees(void);
> @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
>  #define audit_tree_path(rule) ""       /* never called */
>  #define audit_kill_trees(context) BUG()
>
> -#define audit_signal_info(s, t) AUDIT_DISABLED
> +#define audit_signal_info_syscall(t) AUDIT_OFF

Similar as above.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-18 14:59 ` Paul Moore
@ 2019-04-18 15:16   ` Richard Guy Briggs
  2019-04-18 15:37     ` Paul Moore
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Guy Briggs @ 2019-04-18 15:16 UTC (permalink / raw)
  To: Paul Moore
  Cc: LKML, Linux-Audit Mailing List, sgrubb, omosnace, Eric Paris,
	ebiederm, oleg

On 2019-04-18 10:59, Paul Moore wrote:
> On Mon, Apr 8, 2019 at 11:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > When a process signals the audit daemon (shutdown, rotate, resume,
> > reconfig) but syscall auditing is not enabled, we still want to know the
> > identity of the process sending the signal to the audit daemon.
> >
> > Move audit_signal_info() out of syscall auditing to general auditing but
> > create a new function audit_signal_info_syscall() to take care of the
> > syscall dependent parts for when syscall auditing is enabled.
> >
> > Please see the github kernel audit issue
> > https://github.com/linux-audit/audit-kernel/issues/111
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h |  6 ++++++
> >  kernel/audit.c        | 27 +++++++++++++++++++++++++++
> >  kernel/audit.h        |  4 ++--
> >  kernel/auditsc.c      | 19 +++----------------
> >  kernel/signal.c       |  2 +-
> >  5 files changed, 39 insertions(+), 19 deletions(-)
> 
> ...
> 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 1e69d9fe16da..4a22fc3f824f 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -226,6 +229,9 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
> >  }
> >
> >  #define audit_enabled AUDIT_OFF
> > +
> > +#define audit_signal_info(s, t) AUDIT_OFF
> > +
> 
> Should this be AUDIT_DISABLED to preserve the current value/behavior?
> Technically they should both have a value of zero right now, but since
> the AUDIT_DISABLED value isn't explicit it seems safer to go with
> AUDIT_DISABLED.

I did that first, but that symbol was not available when one or both of
CONFIG_AUDITSYSCALL or CONFIG_AUDIT was off, so I had to change it to
AUDIT_OFF.  I followed the logic to confirm that is what was intended by
the original code.

When auidit is off, we want to just return zero so it gets skipped
rather than throwing an error.

> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 958d5b8fc1b3..18a8ae812e9f 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk *chunk,
> >  extern void audit_put_tree(struct audit_tree *tree);
> >  extern void audit_kill_trees(struct audit_context *context);
> >
> > -extern int audit_signal_info(int sig, struct task_struct *t);
> > +extern int audit_signal_info_syscall(struct task_struct *t);
> >  extern void audit_filter_inodes(struct task_struct *tsk,
> >                                 struct audit_context *ctx);
> >  extern struct list_head *audit_killed_trees(void);
> > @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
> >  #define audit_tree_path(rule) ""       /* never called */
> >  #define audit_kill_trees(context) BUG()
> >
> > -#define audit_signal_info(s, t) AUDIT_DISABLED
> > +#define audit_signal_info_syscall(t) AUDIT_OFF
> 
> Similar as above.
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-18 15:16   ` Richard Guy Briggs
@ 2019-04-18 15:37     ` Paul Moore
  2019-04-18 15:42       ` Richard Guy Briggs
  0 siblings, 1 reply; 12+ messages in thread
From: Paul Moore @ 2019-04-18 15:37 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: LKML, Linux-Audit Mailing List, sgrubb, omosnace, Eric Paris,
	ebiederm, oleg

On Thu, Apr 18, 2019 at 11:16 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-04-18 10:59, Paul Moore wrote:
> > On Mon, Apr 8, 2019 at 11:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > When a process signals the audit daemon (shutdown, rotate, resume,
> > > reconfig) but syscall auditing is not enabled, we still want to know the
> > > identity of the process sending the signal to the audit daemon.
> > >
> > > Move audit_signal_info() out of syscall auditing to general auditing but
> > > create a new function audit_signal_info_syscall() to take care of the
> > > syscall dependent parts for when syscall auditing is enabled.
> > >
> > > Please see the github kernel audit issue
> > > https://github.com/linux-audit/audit-kernel/issues/111
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  include/linux/audit.h |  6 ++++++
> > >  kernel/audit.c        | 27 +++++++++++++++++++++++++++
> > >  kernel/audit.h        |  4 ++--
> > >  kernel/auditsc.c      | 19 +++----------------
> > >  kernel/signal.c       |  2 +-
> > >  5 files changed, 39 insertions(+), 19 deletions(-)
> >
> > ...
> >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 1e69d9fe16da..4a22fc3f824f 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -226,6 +229,9 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
> > >  }
> > >
> > >  #define audit_enabled AUDIT_OFF
> > > +
> > > +#define audit_signal_info(s, t) AUDIT_OFF
> > > +
> >
> > Should this be AUDIT_DISABLED to preserve the current value/behavior?
> > Technically they should both have a value of zero right now, but since
> > the AUDIT_DISABLED value isn't explicit it seems safer to go with
> > AUDIT_DISABLED.
>
> I did that first, but that symbol was not available when one or both of
> CONFIG_AUDITSYSCALL or CONFIG_AUDIT was off, so I had to change it to
> AUDIT_OFF.  I followed the logic to confirm that is what was intended by
> the original code.
>
> When auidit is off, we want to just return zero so it gets skipped
> rather than throwing an error.

I understand the desire to return zero in that case, I'm not arguing
against that, I'm just not really in love with how these are defined
when CONFIG_AUDIT isn't.  Part of it is the AUDIT_DISABLED/AUDIT_OFF
change, part of it is the function being defined as a cpp macro
instead of a dummy function (this of course predates your change).
Based on other comments in this thread it looks like you're looking
into a few things and will likely be respinning this patch, since that
is the case, I would prefer if you changed this to just using simply
"0" as opposed to AUDIT_OFF.

If you really want to make me happy about this patch, you would also
change this to a dummy function instead of a cpp macro.  This is a
style nit, and isn't strictly necessary, but I would appreciate it :)

> > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > index 958d5b8fc1b3..18a8ae812e9f 100644
> > > --- a/kernel/audit.h
> > > +++ b/kernel/audit.h
> > > @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk *chunk,
> > >  extern void audit_put_tree(struct audit_tree *tree);
> > >  extern void audit_kill_trees(struct audit_context *context);
> > >
> > > -extern int audit_signal_info(int sig, struct task_struct *t);
> > > +extern int audit_signal_info_syscall(struct task_struct *t);
> > >  extern void audit_filter_inodes(struct task_struct *tsk,
> > >                                 struct audit_context *ctx);
> > >  extern struct list_head *audit_killed_trees(void);
> > > @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
> > >  #define audit_tree_path(rule) ""       /* never called */
> > >  #define audit_kill_trees(context) BUG()
> > >
> > > -#define audit_signal_info(s, t) AUDIT_DISABLED
> > > +#define audit_signal_info_syscall(t) AUDIT_OFF
> >
> > Similar as above.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall
  2019-04-18 15:37     ` Paul Moore
@ 2019-04-18 15:42       ` Richard Guy Briggs
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Guy Briggs @ 2019-04-18 15:42 UTC (permalink / raw)
  To: Paul Moore
  Cc: LKML, Linux-Audit Mailing List, sgrubb, omosnace, Eric Paris,
	ebiederm, oleg

On 2019-04-18 11:37, Paul Moore wrote:
> On Thu, Apr 18, 2019 at 11:16 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-04-18 10:59, Paul Moore wrote:
> > > On Mon, Apr 8, 2019 at 11:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > When a process signals the audit daemon (shutdown, rotate, resume,
> > > > reconfig) but syscall auditing is not enabled, we still want to know the
> > > > identity of the process sending the signal to the audit daemon.
> > > >
> > > > Move audit_signal_info() out of syscall auditing to general auditing but
> > > > create a new function audit_signal_info_syscall() to take care of the
> > > > syscall dependent parts for when syscall auditing is enabled.
> > > >
> > > > Please see the github kernel audit issue
> > > > https://github.com/linux-audit/audit-kernel/issues/111
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  include/linux/audit.h |  6 ++++++
> > > >  kernel/audit.c        | 27 +++++++++++++++++++++++++++
> > > >  kernel/audit.h        |  4 ++--
> > > >  kernel/auditsc.c      | 19 +++----------------
> > > >  kernel/signal.c       |  2 +-
> > > >  5 files changed, 39 insertions(+), 19 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index 1e69d9fe16da..4a22fc3f824f 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -226,6 +229,9 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
> > > >  }
> > > >
> > > >  #define audit_enabled AUDIT_OFF
> > > > +
> > > > +#define audit_signal_info(s, t) AUDIT_OFF
> > > > +
> > >
> > > Should this be AUDIT_DISABLED to preserve the current value/behavior?
> > > Technically they should both have a value of zero right now, but since
> > > the AUDIT_DISABLED value isn't explicit it seems safer to go with
> > > AUDIT_DISABLED.
> >
> > I did that first, but that symbol was not available when one or both of
> > CONFIG_AUDITSYSCALL or CONFIG_AUDIT was off, so I had to change it to
> > AUDIT_OFF.  I followed the logic to confirm that is what was intended by
> > the original code.
> >
> > When auidit is off, we want to just return zero so it gets skipped
> > rather than throwing an error.
> 
> I understand the desire to return zero in that case, I'm not arguing
> against that, I'm just not really in love with how these are defined
> when CONFIG_AUDIT isn't.  Part of it is the AUDIT_DISABLED/AUDIT_OFF
> change, part of it is the function being defined as a cpp macro
> instead of a dummy function (this of course predates your change).
> Based on other comments in this thread it looks like you're looking
> into a few things and will likely be respinning this patch, since that
> is the case, I would prefer if you changed this to just using simply
> "0" as opposed to AUDIT_OFF.
> 
> If you really want to make me happy about this patch, you would also
> change this to a dummy function instead of a cpp macro.  This is a
> style nit, and isn't strictly necessary, but I would appreciate it :)

In fact, I'd started with a dummy function, then changed it to match the
style of the rest of the file when I got a pile of unused function
complaints from all over the kernel...  I'll see what I can do to go
back to the dummy function.

> > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > index 958d5b8fc1b3..18a8ae812e9f 100644
> > > > --- a/kernel/audit.h
> > > > +++ b/kernel/audit.h
> > > > @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk *chunk,
> > > >  extern void audit_put_tree(struct audit_tree *tree);
> > > >  extern void audit_kill_trees(struct audit_context *context);
> > > >
> > > > -extern int audit_signal_info(int sig, struct task_struct *t);
> > > > +extern int audit_signal_info_syscall(struct task_struct *t);
> > > >  extern void audit_filter_inodes(struct task_struct *tsk,
> > > >                                 struct audit_context *ctx);
> > > >  extern struct list_head *audit_killed_trees(void);
> > > > @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
> > > >  #define audit_tree_path(rule) ""       /* never called */
> > > >  #define audit_kill_trees(context) BUG()
> > > >
> > > > -#define audit_signal_info(s, t) AUDIT_DISABLED
> > > > +#define audit_signal_info_syscall(t) AUDIT_OFF
> > >
> > > Similar as above.
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-04-18 15:42 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-09  3:52 [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall Richard Guy Briggs
2019-04-09  6:01 ` Steve Grubb
2019-04-09 14:02   ` Richard Guy Briggs
2019-04-09 15:37     ` Steve Grubb
2019-04-09 15:57       ` Richard Guy Briggs
2019-04-10  0:25         ` Eric W. Biederman
2019-04-10 16:54           ` Richard Guy Briggs
2019-04-11 12:22         ` Steve Grubb
2019-04-18 14:59 ` Paul Moore
2019-04-18 15:16   ` Richard Guy Briggs
2019-04-18 15:37     ` Paul Moore
2019-04-18 15:42       ` Richard Guy Briggs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).