Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley <peter@hurleysoftware.com>]

On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> The tty field was missing from AUDIT_LOGIN events.
> 
> Refactor code to create a new function audit_get_tty(), using it to
> replace the call in audit_log_task_info() and to add it to
> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> audit_put_tty() alias to decrement it.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> 
> V4: Add missing prototype for audit_put_tty() when audit syscall is not
>     enabled (MIPS).
> 
> V3: Introduce audit_put_tty() alias to decrement kref.
> 
> V2: Use kref to protect tty signal struct while in use.
> 
> ---
> 
>  include/linux/audit.h |   24 ++++++++++++++++++++++++
>  kernel/audit.c        |   18 +++++-------------
>  kernel/auditsc.c      |    8 ++++++--
>  3 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b40ed5d..32cdafb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>  #include <linux/sched.h>
>  #include <linux/ptrace.h>
>  #include <uapi/linux/audit.h>
> +#include <linux/tty.h>
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>  	return tsk->sessionid;
>  }
>  
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> +	struct tty_struct *tty = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsk->sighand->siglock, flags);
> +	if (tsk->signal)
> +		tty = tty_kref_get(tsk->signal->tty);
> +	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);


Not that I'm objecting because I get that you're just refactoring
existing code, but I thought I'd point out some stuff.

1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
   because if it is, this will blow up trying to dereference the
   sighand_struct (ie tsk->sighand).

2. The existing usage is always tsk==current

3. If the idea is to make this invulnerable to tsk being gone, then
   the usage is unsafe anyway.


So ultimately (but not necessarily for this patch) I'd prefer that either
a. audit use existing tty api instead of open-coding, or
b. add any tty api functions required.


Regards,
Peter Hurley


> +	return tty;
> +}
> +
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{
> +	tty_kref_put(tty);
> +}
> +
>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
>  extern void __audit_bprm(struct linux_binprm *bprm);
> @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>  {
>  	return -1;
>  }
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> +	return NULL;
> +}
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{ }
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  { }
>  static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3a3e5de..7edd776 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -64,7 +64,6 @@
>  #include <linux/security.h>
>  #endif
>  #include <linux/freezer.h>
> -#include <linux/tty.h>
>  #include <linux/pid_namespace.h>
>  #include <net/netns/generic.h>
>  
> @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
>  {
>  	const struct cred *cred;
>  	char comm[sizeof(tsk->comm)];
> -	char *tty;
> +	struct tty_struct *tty;
>  
>  	if (!ab)
>  		return;
>  
>  	/* tsk == current */
>  	cred = current_cred();
> -
> -	spin_lock_irq(&tsk->sighand->siglock);
> -	if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)
> -		tty = tsk->signal->tty->name;
> -	else
> -		tty = "(none)";
> -	spin_unlock_irq(&tsk->sighand->siglock);
> -
> +	tty = audit_get_tty(tsk);
>  	audit_log_format(ab,
>  			 " ppid=%d pid=%d auid=%u uid=%u gid=%u"
>  			 " euid=%u suid=%u fsuid=%u"
> @@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
>  			 from_kgid(&init_user_ns, cred->egid),
>  			 from_kgid(&init_user_ns, cred->sgid),
>  			 from_kgid(&init_user_ns, cred->fsgid),
> -			 tty, audit_get_sessionid(tsk));
> -
> +			 tty ? tty_name(tty) : "(none)",
> +			 audit_get_sessionid(tsk));
> +	audit_put_tty(tty);
>  	audit_log_format(ab, " comm=");
>  	audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
> -
>  	audit_log_d_path_exe(ab, tsk->mm);
>  	audit_log_task_context(ab);
>  }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 195ffae..71e14d8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
>  {
>  	struct audit_buffer *ab;
>  	uid_t uid, oldloginuid, loginuid;
> +	struct tty_struct *tty;
>  
>  	if (!audit_enabled)
>  		return;
> @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
>  	uid = from_kuid(&init_user_ns, task_uid(current));
>  	oldloginuid = from_kuid(&init_user_ns, koldloginuid);
>  	loginuid = from_kuid(&init_user_ns, kloginuid),
> +	tty = audit_get_tty(current);
>  
>  	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
>  	if (!ab)
>  		return;
>  	audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
>  	audit_log_task_context(ab);
> -	audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u res=%d",
> -			 oldloginuid, loginuid, oldsessionid, sessionid, !rc);
> +	audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u res=%d",
> +			 oldloginuid, loginuid, tty ? tty_name(tty) : "(none)",
> +			 oldsessionid, sessionid, !rc);
> +	audit_put_tty(tty);
>  	audit_log_end(ab);
>  }
>  
>