Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753000AbcDZWeo (ORCPT ); Tue, 26 Apr 2016 18:34:44 -0400 Received: from mail-oi0-f53.google.com ([209.85.218.53]:33928 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752362AbcDZWem (ORCPT ); Tue, 26 Apr 2016 18:34:42 -0400 MIME-Version: 1.0 X-Originating-IP: [108.49.39.189] In-Reply-To: <571A5C54.7050704@hurleysoftware.com> References: <571A5C54.7050704@hurleysoftware.com> Date: Tue, 26 Apr 2016 18:34:41 -0400 Message-ID: Subject: Re: [PATCH V4] audit: add tty field to LOGIN event From: Paul Moore To: Peter Hurley , Richard Guy Briggs Cc: linux-audit@redhat.com, Paul Moore , linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2360 Lines: 64 On Fri, Apr 22, 2016 at 1:16 PM, Peter Hurley 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 >> #include >> #include >> +#include >> >> #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