Re: [PATCH V4] audit: add tty field to LOGIN event
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? Feel
free to elaborate by patch too ;)
--
paul moore
www.paul-moore.com