Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752804AbcD1T2J (ORCPT ); Thu, 28 Apr 2016 15:28:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42061 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752309AbcD1T2H (ORCPT ); Thu, 28 Apr 2016 15:28:07 -0400 Date: Thu, 28 Apr 2016 15:28:03 -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: <20160428192803.GG18994@madcap2.tricolour.ca> References: <571A5C54.7050704@hurleysoftware.com> <20160428013140.GD18994@madcap2.tricolour.ca> <57217DDF.5070608@hurleysoftware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57217DDF.5070608@hurleysoftware.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 28 Apr 2016 19:28:06 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8507 Lines: 246 On 16/04/27, Peter Hurley wrote: > On 04/27/2016 06:31 PM, Richard Guy Briggs wrote: > > 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. > > While it's true that tsk != current here, everything relevant to tty > in task_struct is the same because the nascent task is not even half-done. > So tsk->sighand == current->sighand, tsk->signal == current->signal etc. I agree this is true except in the case of !CLONE_SIGHAND, if it fails after copy_sighand() or copy_signal() then it would be null and would get freed before audit_free() is called. By the time tty gets copied from current in this case, it is past the point of failure in copy_process(). > If you're uncomfortable with pass-through execution like that, then the > simple solution is: > > struct tty_struct *tty = NULL; > > /* tsk != current when copy_process() failed */ > if (tsk == current) > tty = get_current_tty(); > > because tty_kref_put(tty) accepts NULL tty and (obviously) so does > tty_name(tty). Given the circumstances above, this appears reasonable to me at first look. > Peter Hurley > > > 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 > > > - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635