Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752093AbcDRS1G (ORCPT ); Mon, 18 Apr 2016 14:27:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48087 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751565AbcDRS1D (ORCPT ); Mon, 18 Apr 2016 14:27:03 -0400 Date: Mon, 18 Apr 2016 14:27:00 -0400 From: Richard Guy Briggs To: Peter Hurley Cc: linux-audit@redhat.com, linux-kernel@vger.kernel.org, sgrubb@redhat.com, pmoore@redhat.com, eparis@redhat.com Subject: Re: [PATCH] audit: add tty field to LOGIN event Message-ID: <20160418182700.GD4577@madcap2.tricolour.ca> References: <4587fd4a69c5d41f9596c0644ce22dc38db47d04.1460589810.git.rgb@redhat.com> <570EE4D4.4080903@hurleysoftware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <570EE4D4.4080903@hurleysoftware.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5082 Lines: 165 On 16/04/13, Peter Hurley wrote: > Hi Richard, Hi Peter, > On 04/13/2016 04:25 PM, 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(). > > > > Signed-off-by: Richard Guy Briggs > > --- > > include/linux/audit.h | 18 ++++++++++++++++++ > > kernel/audit.c | 11 +---------- > > kernel/auditsc.c | 5 +++-- > > 3 files changed, 22 insertions(+), 12 deletions(-) > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index b40ed5d..20c6649 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -26,6 +26,7 @@ > > #include > > #include > > #include > > +#include > > > > #define AUDIT_INO_UNSET ((unsigned long)-1) > > #define AUDIT_DEV_UNSET ((dev_t)-1) > > @@ -343,6 +344,19 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk) > > return tsk->sessionid; > > } > > > > +static inline char *audit_get_tty(struct task_struct *tsk) > > +{ > > + char *tty; > > + > > + 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); > > This is unsafe because the tty could be immediately torn down after the > siglock is dropped, and return a dangling ptr. Understood. The other option is to copy the value out... Thanks for the helpful review. Rev 2 coming... > > + return 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 +514,10 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk) > > { > > return -1; > > } > > +static inline char *audit_get_tty(struct task_struct *tsk) > > +{ > > + return "(invalid)"; > > +} > > 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..fae11df 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -64,7 +64,6 @@ > > #include > > #endif > > #include > > -#include > > #include > > #include > > > > @@ -1873,7 +1872,6 @@ 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; > > @@ -1881,13 +1879,6 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) > > /* 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 = get_current_tty(); > > > - > > audit_log_format(ab, > > " ppid=%d pid=%d auid=%u uid=%u gid=%u" > > " euid=%u suid=%u fsuid=%u" > > @@ -1903,7 +1894,7 @@ 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)); > > + audit_get_tty(tsk), audit_get_sessionid(tsk)); > > tty_name(tty), ....); > ^^^^^^^^^^ > returns "NULL tty" if tty == NULL > > tty_kref_put(tty); > > > > > > audit_log_format(ab, " comm="); > > audit_log_untrustedstring(ab, get_task_comm(comm, tsk)); > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 195ffae..a0467fb 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -1993,8 +1993,9 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid, > > 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); > > tty = get_current_tty(); > > > + audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u res=%d", > > + oldloginuid, loginuid, audit_get_tty(current), > > ......., tty_name(tty), > > > + oldsessionid, sessionid, !rc); > > tty_kref_put(tty); > > Regards, > Peter Hurley > > > > audit_log_end(ab); > > } > > > > > - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635