Subject: Re: [PATCH V4] audit: add tty field to LOGIN event
To: Paul Moore <paul@paul-moore.com>, Richard Guy Briggs <rgb@redhat.com>
References: <dedf98fab21b8bd8bc96a1200ba1c3c1a105324f.1461260693.git.rgb@redhat.com>
 <571A5C54.7050704@hurleysoftware.com>
 <CAHC9VhTiY5s19_W22KpzMTrQG2Y0TethjTxeWFnSFY9PCNNwzg@mail.gmail.com>
Cc: linux-audit@redhat.com, Paul Moore <pmoore@redhat.com>,
        linux-kernel@vger.kernel.org
From: Peter Hurley <peter@hurleysoftware.com>
Message-ID: <57200E94.8010306@hurleysoftware.com>
Date: Tue, 26 Apr 2016 17:57:56 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <CAHC9VhTiY5s19_W22KpzMTrQG2Y0TethjTxeWFnSFY9PCNNwzg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2780
Lines: 74

On 04/26/2016 03:34 PM, Paul Moore wrote:
> On Fri, Apr 22, 2016 at 1:16 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
>>> 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);
> 
> I just merged Richard's patch, if nothing else it is better than it
> was.  However, I would like to talk about improving things, see below.
> 
>> 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).
>>
>> 2. The existing usage is always tsk==current
> 
> Yep, there is only one caller I found that even works on task_structs
> other than current (see audit_log_exit() via audit_free()), although
> even then when it ends up calling into audit_log_task_info() tsk
> should always be current.
> 
> I've got a patch compiling now to get rid of passing around current as
> a a task_struct argument, assuming nothing blows up in testing I'll
> post/merge it.
> 
>> 3. If the idea is to make this invulnerable to tsk being gone, then
>>    the usage is unsafe anyway.
> 
> I don't think that is our concern here.
> 
>> 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.
> 
> I'm open to suggestions, care to elaborate on either option?

So b) is only necessary if the answer to 3) was yes or if tsk != current.
Otherwise, the new audit_get_tty() is equivalent to get_current_tty()
which is the exported tty core interface for the identical operation.

I was only suggesting b) if get_current_tty() wasn't going to be
sufficient.

> Feel free to elaborate by patch too ;)

I can do that.

Regards,
Peter Hurley