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

On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > > 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); > + 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?

> 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.

paul moore