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

On 16/04/22, Peter Hurley wrote: > On 04/21/2016 11:14 AM, Richard Guy Briggs 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); > > > 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). Ok. This logic goes back 10 years and one month less two days. (45d9bb0e) > 2. The existing usage is always tsk==current My understanding is that when it is called via: copy_process() audit_free() __audit_free() audit_log_exit() audit_log_task_info() then tsk != current.

This appears to be the only case which appears to force lugging around tsk. This is noted in that commit referenced above. > 3. If the idea is to make this invulnerable to tsk being gone, then > the usage is unsafe anyway. > > > 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. This latter option did cross my mind... > Peter Hurley > >> + return tty; >> +} >> + >> +static inline void audit_put_tty(struct tty_struct *tty) >> +{ >> + tty_kref_put(tty); >> +} >> + >> extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp); >> extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode); >> extern void __audit_bprm(struct linux_binprm *bprm); >> @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk) >> { >> return -1; >> } >> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk) >> +{ >> + return NULL; >> +} >> +static inline void audit_put_tty(struct tty_struct *tty) >> +{ } >> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) >> { } >> static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid, >> diff --git a/kernel/audit.c b/kernel/audit.c >> index 3a3e5de..7edd776 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -64,7 +64,6 @@ >> #include <linux/security.h> >> #endif >> #include <linux/freezer.h> >> -#include <linux/tty.h> >> #include <linux/pid_namespace.h> >> #include <net/netns/generic.h> >> >> @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) >> { >> const struct cred *cred; >> char comm[sizeof(tsk->comm)]; >> - char *tty; >> + struct tty_struct *tty; >> >> if (!ab) >> return; >> >> /* tsk == current */ >> cred = current_cred(); >> - >> - spin_lock_irq(&tsk->sighand->siglock); >> - if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name) >> - tty = tsk->signal->tty->name; >> - else >> - tty = "(none)"; >> - spin_unlock_irq(&tsk->sighand->siglock); >> - >> + tty = audit_get_tty(tsk); >> audit_log_format(ab, >> " ppid=%d pid=%d auid=%u uid=%u gid=%u" >> " euid=%u suid=%u fsuid=%u" >> @@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) >> from_kgid(&init_user_ns, cred->egid), >> from_kgid(&init_user_ns, cred->sgid), >> from_kgid(&init_user_ns, cred->fsgid), >> - tty, audit_get_sessionid(tsk)); >> - >> + tty ? tty_name(tty) : "(none)", >> + audit_get_sessionid(tsk)); >> + audit_put_tty(tty); >> audit_log_format(ab, " comm="); >> audit_log_untrustedstring(ab, get_task_comm(comm, tsk)); >> - >> audit_log_d_path_exe(ab, tsk->mm); >> audit_log_task_context(ab); >> } >> 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); >> } >> >> > - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635