Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752522AbcD1Bbq (ORCPT ); Wed, 27 Apr 2016 21:31:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35931 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752236AbcD1Bbp (ORCPT ); Wed, 27 Apr 2016 21:31:45 -0400 Date: Wed, 27 Apr 2016 21:31:40 -0400 From: Richard Guy Briggs To: Peter Hurley Cc: linux-audit@redhat.com, pmoore@redhat.com, linux-kernel@vger.kernel.org, sgrubb@redhat.com, eparis@redhat.com Subject: Re: [PATCH V4] audit: add tty field to LOGIN event Message-ID: <20160428013140.GD18994@madcap2.tricolour.ca> References: <571A5C54.7050704@hurleysoftware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <571A5C54.7050704@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: 6823 Lines: 205 On 16/04/22, Peter Hurley wrote: > 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 > > --- > > > > 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 > > #include > > #include > > +#include > > > > #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). Ok. This logic goes back 10 years and one month less two days. (45d9bb0e) > 2. The existing usage is always tsk==current My understanding is that when it is called via: copy_process() audit_free() __audit_free() audit_log_exit() audit_log_task_info() then tsk != current. This appears to be the only case which appears to force lugging around tsk. This is noted in that commit referenced above. > 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. This latter option did cross my mind... > 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 > > #endif > > #include > > -#include > > #include > > #include > > > > @@ -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); > > } > > > > > - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635