Re: [PATCH V4] audit: add tty field to LOGIN event
On 04/26/2016 03:34 PM, Paul Moore wrote:
On Fri, Apr 22, 2016 at 1:16 PM, Peter Hurley
<peter(a)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