Date:	Fri, 22 Apr 2016 11:02:48 -0400
From:	Paul Moore <paul@...l-moore.com>
To:	Richard Guy Briggs <rgb@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	peter@...leysoftware.com
Subject: Re: [PATCH V4] audit: add tty field to LOGIN event

On Thu, Apr 21, 2016 at 11:50 PM, Richard Guy Briggs <rgb@...hat.com> wrote:
> On 16/04/21, Paul Moore wrote:
>> On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs <rgb@...hat.com> 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@...hat.com>
>> > ---
>> >
>> > 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 did reflect on that initially and decided this was the least
> disruptive way to implement it.  There are others similar around it and
> when I started it wasn't as involved, but now it is starting to push the
> limits with the kref bits...

Yeah, that is why I mentioned it, it is sort of borderline in my
opinion of what I would consider acceptable in a header file, barring
some special case.

>> I guess I'm fine with this, but what was the idea behind the static
>> inlines in audit.h?  Performance, or something else?
>
> Can't say I remember...  I was tempted to put it in as a define, but
> decided to stick with the existing style, right?  :-)

No, definitely not a cpp macro, we'd lose type checking and other
stuff.  My debate is whether or not this should life fully in the
header file vs simply a prototype in the header and the definition in
a C file.  This is the first function where we are actually putting
content into linux/audit.h, all of the existing function definitions
there are dummy functions or simple wrappers for performance reasons.

>> > 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.
>
> Well, I did try to put that keyword consistently with where it was
> inserted in other messages.  My understanding was that adding an extra
> in the middle wouldn't cause a problem because the keyword scanner
> looked ahead until it found the keyword it sought.  This way, older
> scanners will just hop over this keyword it wasn't seeking.

I understand the idea behind consistency, and as long as it doesn't
break the userspace parser(s) I agree with you.  The good news is that
Steve says we are in the clear wrt the new field.

I'm traveling right now and I'm not sure if I'll have any time in
front of a proper computer/shell to get this merged before early next
week, but v4 looks reasonable to me.  If you don't see this in the
audit#next tree by .... let's say end of day next Tuesday ... feel
free to send me a nudge.

Thanks.

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists