Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753816AbcDVNNR (ORCPT ); Fri, 22 Apr 2016 09:13:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44656 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753776AbcDVNNO (ORCPT ); Fri, 22 Apr 2016 09:13:14 -0400 From: Steve Grubb To: linux-audit@redhat.com Cc: Paul Moore , Richard Guy Briggs , linux-kernel@vger.kernel.org, peter@hurleysoftware.com Subject: Re: [PATCH V4] audit: add tty field to LOGIN event Date: Fri, 22 Apr 2016 09:13:18 -0400 Message-ID: <2689878.2iyKlAmhn2@x2> Organization: Red Hat User-Agent: KMail/4.14.10 (Linux/4.4.7-300.fc23.x86_64; KDE/4.14.18; x86_64; ; ) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 22 Apr 2016 13:13:14 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4429 Lines: 129 On Thursday, April 21, 2016 09:29:57 PM Paul Moore wrote: > On Thu, Apr 21, 2016 at 2:14 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(). 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); > > + return tty; > > +} > > + > > +static inline void audit_put_tty(struct tty_struct *tty) > > +{ > > + tty_kref_put(tty); > > +} > > I'm generally not a big fan of defining functions as inlines in header > files, with the general exception of dummy functions that will get > compiled out. Although I guess there might be some performance > argument to be made wrt audit_log_task_info(). > > I guess I'm fine with this, but what was the idea behind the static > inlines in audit.h? Performance, or something else? I think that is normal to prevent multiple definitions at link time. > > 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); > > > > } > > Because we are still using the crappy fixed string format for > kernel<->userspace communication, this patch will likely "break the > world" ... let's check with Steve but I believe the way to handle this > is to add the tty information to the end of the record. The placement is OK. Thanks for asking. -Steve