Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp915624imu; Fri, 4 Jan 2019 09:25:15 -0800 (PST) X-Google-Smtp-Source: ALg8bN5mmm2hnkmzfvmGeXJ/RuZWP0XaNNcbQWYGCsk+5PK8FbgQ2fLAd+YoOuGgbgH2dFSL4HSD X-Received: by 2002:a63:a41:: with SMTP id z1mr2356636pgk.117.1546622715176; Fri, 04 Jan 2019 09:25:15 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1546622715; cv=none; d=google.com; s=arc-20160816; b=D/DQqdNRknBlowC6/vxUrmh/2QyVJjGahkmleHRfweY6l5tCYFZ2mf1A4C1Shf1CjH m8YiXU7XUFuMugWJmziiq6d7abspLntTUkmYG3scx8dPJiEjk/vKC3sWZKOVl/Adt6nC WP6vQHp8I3S2dEu7LgIa9R9DOY1+tx7XtRVSxTiIm8DECD64RA3DX+sAqyBF0gpSOO9h si22Ofb41NdJ60ztVC2ptOpB/DVM+JKcSQDHub2osbZfHYEZMIcN7throUnHn9gVKoiY OtjmxiJr0b+RS41/GylXdIb3++wVcnx5wRNqiceVK0zLVr7pz+eXdCQG2Lnx1RMK31p4 3+dA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=m6qkKmdmxCEc6Iq/U0mGtg/WyhHWP1VSMMjpSim8/Pc=; b=QU9DiyCI6L69eU/ofthaEwmKmrzt0a3eQmm8sf+LpUHSle+EIcZ90kOS96z+CR59dT Z3BEe+ahAQ/rU1pZl6u6UGe4Ejqqf38BKTwEwZHtDbHRiWxgCfdmTsIW1wBpZOwR3dr8 iOiC6kMW7ZXJaEvwADxjvaa/ZliQN7yVgI/qEaX6pSg6mLbTJbnZ10JptVEwtKyVDG8A Yaqi2IhrKy6OG7iVOjyIKM4HpzbI28EdN9UWhu7JBWQ3yghGzI4AS8p0Haj7kgNUPEnu Jfw6VwhcFLj/TVz5XEYGFTynh9PZ11eOKG6fgCxBJf4rSY1ZGLzXKG6eddxVuSTpifUk F6Aw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w12si56618506pfn.212.2019.01.04.09.24.59; Fri, 04 Jan 2019 09:25:15 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726757AbfADO5z (ORCPT + 99 others); Fri, 4 Jan 2019 09:57:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37902 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726117AbfADO5y (ORCPT ); Fri, 4 Jan 2019 09:57:54 -0500 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6069DA7877; Fri, 4 Jan 2019 14:57:53 +0000 (UTC) Received: from madcap2.tricolour.ca (ovpn-112-10.phx2.redhat.com [10.3.112.10]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9A817600D6; Fri, 4 Jan 2019 14:57:38 +0000 (UTC) Date: Fri, 4 Jan 2019 09:57:35 -0500 From: Richard Guy Briggs To: Guenter Roeck Cc: containers@lists.linux-foundation.org, linux-api@vger.kernel.org, Linux-Audit Mailing List , linux-fsdevel@vger.kernel.org, LKML , netdev@vger.kernel.org, netfilter-devel@vger.kernel.org, ebiederm@xmission.com, luto@kernel.org, carlos@redhat.com, dhowells@redhat.com, viro@zeniv.linux.org.uk, simo@redhat.com, eparis@parisplace.org, serge@hallyn.com Subject: Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters Message-ID: <20190104145735.vlrw2fip5syn2exc@madcap2.tricolour.ca> References: <8e617ab568df28a66dfbe3284452de186b42fb0f.1533065887.git.rgb@redhat.com> <20190104025006.GA15567@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190104025006.GA15567@roeck-us.net> User-Agent: NeoMutt/20180716 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 04 Jan 2019 14:57:53 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2019-01-03 18:50, Guenter Roeck wrote: > Hi Richard, > > On Tue, Jul 31, 2018 at 04:07:36PM -0400, Richard Guy Briggs wrote: > > The audit-related parameters in struct task_struct should ideally be > > collected together and accessed through a standard audit API. > > > > Collect the existing loginuid, sessionid and audit_context together in a > > new struct audit_task_info called "audit" in struct task_struct. > > > > Use kmem_cache to manage this pool of memory. > > Un-inline audit_free() to be able to always recover that memory. > > > > See: https://github.com/linux-audit/audit-kernel/issues/81 > > > > Signed-off-by: Richard Guy Briggs > > Overall I am not sure if keeping task_struct a bit smaller is worth > the added complexity, but I guess that is just me. The motivation was to consolidate all the audit bits into one pointer, isolating them from the rest of the kernel, restricting access only to helper functions to prevent abuse by other subsystems and trying to reduce kABI issues in the future. I agree it is a bit more complex. It was provoked by the need to add contid which seemed to make the most sense as a peer to loginuid and sessionid, and adding it to task_struct would have made it a bit too generic and available. This is addressed at some length by Paul Moore here in v2: https://lkml.org/lkml/2018/4/18/759 > Anyway, couple of nitpicks. Please feel free to ignore, and my apologies > if some of all of the comments are duplicates. Noted. They all look like reasonable improvements, particulaly the unnecessary else and default return. Thanks. The double context check may go away anyways based on the removal of audit_take_context() in Paul's 2a1fe215e730 ("audit: use current whenever possible") which has yet to be incorporated. > Guenter > > > --- > > include/linux/audit.h | 34 ++++++++++++++++++++++++---------- > > include/linux/sched.h | 5 +---- > > init/init_task.c | 3 +-- > > init/main.c | 2 ++ > > kernel/auditsc.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- > > kernel/fork.c | 4 +++- > > 6 files changed, 73 insertions(+), 26 deletions(-) > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 9334fbe..8964332 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -219,8 +219,15 @@ static inline void audit_log_task_info(struct audit_buffer *ab, > > > > /* These are defined in auditsc.c */ > > /* Public API */ > > Not sure if the structure below belongs after "Public API". > Is it part of the public API ? > > > +struct audit_task_info { > > + kuid_t loginuid; > > + unsigned int sessionid; > > + struct audit_context *ctx; > > +}; > > Add empty line ? > > > +extern struct audit_task_info init_struct_audit; > > +extern void __init audit_task_init(void); > > extern int audit_alloc(struct task_struct *task); > > -extern void __audit_free(struct task_struct *task); > > +extern void audit_free(struct task_struct *task); > > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1, > > unsigned long a2, unsigned long a3); > > extern void __audit_syscall_exit(int ret_success, long ret_value); > > @@ -242,12 +249,15 @@ extern void audit_seccomp_actions_logged(const char *names, > > > > static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx) > > { > > - task->audit_context = ctx; > > + task->audit->ctx = ctx; > > } > > > > static inline struct audit_context *audit_context(void) > > { > > - return current->audit_context; > > + if (current->audit) > > + return current->audit->ctx; > > + else > > + return NULL; > > Unnecessary else (and static checkers may complain). > > > } > > > > static inline bool audit_dummy_context(void) > > @@ -255,11 +265,7 @@ static inline bool audit_dummy_context(void) > > void *p = audit_context(); > > return !p || *(int *)p; > > } > > -static inline void audit_free(struct task_struct *task) > > -{ > > - if (unlikely(task->audit_context)) > > - __audit_free(task); > > -} > > + > > static inline void audit_syscall_entry(int major, unsigned long a0, > > unsigned long a1, unsigned long a2, > > unsigned long a3) > > @@ -331,12 +337,18 @@ extern int auditsc_get_stamp(struct audit_context *ctx, > > > > static inline kuid_t audit_get_loginuid(struct task_struct *tsk) > > { > > - return tsk->loginuid; > > + if (tsk->audit) > > + return tsk->audit->loginuid; > > + else > > + return INVALID_UID; > > Unnecessary else. > > > } > > > > static inline unsigned int audit_get_sessionid(struct task_struct *tsk) > > { > > - return tsk->sessionid; > > + if (tsk->audit) > > + return tsk->audit->sessionid; > > + else > > + return AUDIT_SID_UNSET; > > Unnecessary else. > > > } > > > > extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp); > > @@ -461,6 +473,8 @@ static inline void audit_fanotify(unsigned int response) > > extern int audit_n_rules; > > extern int audit_signals; > > #else /* CONFIG_AUDITSYSCALL */ > > +static inline void __init audit_task_init(void) > > +{ } > > static inline int audit_alloc(struct task_struct *task) > > { > > return 0; > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index 87bf02d..e117272 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -30,7 +30,6 @@ > > #include > > > > /* task_struct member predeclarations (sorted alphabetically): */ > > -struct audit_context; > > struct backing_dev_info; > > struct bio_list; > > struct blk_plug; > > @@ -873,10 +872,8 @@ struct task_struct { > > > > struct callback_head *task_works; > > > > - struct audit_context *audit_context; > > #ifdef CONFIG_AUDITSYSCALL > > - kuid_t loginuid; > > - unsigned int sessionid; > > + struct audit_task_info *audit; > > #endif > > struct seccomp seccomp; > > > > diff --git a/init/init_task.c b/init/init_task.c > > index 74f60ba..4058840 100644 > > --- a/init/init_task.c > > +++ b/init/init_task.c > > @@ -119,8 +119,7 @@ struct task_struct init_task > > .thread_group = LIST_HEAD_INIT(init_task.thread_group), > > .thread_node = LIST_HEAD_INIT(init_signals.thread_head), > > #ifdef CONFIG_AUDITSYSCALL > > - .loginuid = INVALID_UID, > > - .sessionid = AUDIT_SID_UNSET, > > + .audit = &init_struct_audit, > > #endif > > #ifdef CONFIG_PERF_EVENTS > > .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex), > > diff --git a/init/main.c b/init/main.c > > index 3b4ada1..6aba171 100644 > > --- a/init/main.c > > +++ b/init/main.c > > @@ -92,6 +92,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -721,6 +722,7 @@ asmlinkage __visible void __init start_kernel(void) > > nsfs_init(); > > cpuset_init(); > > cgroup_init(); > > + audit_task_init(); > > taskstats_init_early(); > > delayacct_init(); > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index fb20746..88779a7 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -841,7 +841,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk, > > int return_valid, > > long return_code) > > { > > - struct audit_context *context = tsk->audit_context; > > + struct audit_context *context = tsk->audit->ctx; > > > > if (!context) > > return NULL; > > @@ -926,6 +926,15 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state) > > return context; > > } > > > > +static struct kmem_cache *audit_task_cache; > > + > > +void __init audit_task_init(void) > > +{ > > + audit_task_cache = kmem_cache_create("audit_task", > > + sizeof(struct audit_task_info), > > + 0, SLAB_PANIC, NULL); > > +} > > + > > /** > > * audit_alloc - allocate an audit context block for a task > > * @tsk: task > > @@ -940,17 +949,28 @@ int audit_alloc(struct task_struct *tsk) > > struct audit_context *context; > > enum audit_state state; > > char *key = NULL; > > + struct audit_task_info *info; > > + > > + info = kmem_cache_zalloc(audit_task_cache, GFP_KERNEL); > > + if (!info) > > + return -ENOMEM; > > + info->loginuid = audit_get_loginuid(current); > > + info->sessionid = audit_get_sessionid(current); > > + tsk->audit = info; > > > > if (likely(!audit_ever_enabled)) > > return 0; /* Return if not auditing. */ > > > > state = audit_filter_task(tsk, &key); > > if (state == AUDIT_DISABLED) { > > + audit_set_context(tsk, NULL); > > clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT); > > return 0; > > } > > > > if (!(context = audit_alloc_context(state))) { > > + tsk->audit = NULL; > > + kmem_cache_free(audit_task_cache, info); > > kfree(key); > > audit_log_lost("out of memory in audit_alloc"); > > return -ENOMEM; > > @@ -962,6 +982,12 @@ int audit_alloc(struct task_struct *tsk) > > return 0; > > } > > > > +struct audit_task_info init_struct_audit = { > > + .loginuid = INVALID_UID, > > + .sessionid = AUDIT_SID_UNSET, > > + .ctx = NULL, > > +}; > > + > > static inline void audit_free_context(struct audit_context *context) > > { > > audit_free_names(context); > > @@ -1469,26 +1495,33 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts > > } > > > > /** > > - * __audit_free - free a per-task audit context > > + * audit_free - free a per-task audit context > > * @tsk: task whose audit context block to free > > * > > * Called from copy_process and do_exit > > */ > > -void __audit_free(struct task_struct *tsk) > > +void audit_free(struct task_struct *tsk) > > { > > struct audit_context *context; > > + struct audit_task_info *info; > > > > context = audit_take_context(tsk, 0, 0); > > - if (!context) > > - return; > > - > > /* Check for system calls that do not go through the exit > > * function (e.g., exit_group), then free context block. > > * We use GFP_ATOMIC here because we might be doing this > > * in the context of the idle thread */ > > /* that can happen only if we are called from do_exit() */ > > - if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) > > + if (context && context->in_syscall && > > + context->current_state == AUDIT_RECORD_CONTEXT) > > audit_log_exit(context, tsk); > > + /* Freeing the audit_task_info struct must be performed after > > + * audit_log_exit() due to need for loginuid and sessionid. > > + */ > > + info = tsk->audit; > > + tsk->audit = NULL; > > + kmem_cache_free(audit_task_cache, info); > > + if (!context) > > + return; > > if (!list_empty(&context->killed_trees)) > > audit_kill_trees(&context->killed_trees); > > Looks kind of terrible with the repeated check if context is NULL. > Maybe reorder ? > > context = audit_take_context(tsk, 0, 0); > if (context) { > /* do all the context work */ > } > kmem_cache_free(audit_task_cache, tsk->audit); > tsk->audit = NULL; // is that even necessary ? > > If "info" is really needed, ie if tsk (and tsk->audit) can be accessed > from another thread in parallel, I'd be a bit concerned about the lack > of sync() or similar after clearing tsk->audit. > > Another option might have been to separate audit_free() into > audit_free_context() and audit_free_info(). > > > > > @@ -2071,8 +2104,8 @@ int audit_set_loginuid(kuid_t loginuid) > > sessionid = (unsigned int)atomic_inc_return(&session_id); > > } > > > > - task->sessionid = sessionid; > > - task->loginuid = loginuid; > > + task->audit->sessionid = sessionid; > > + task->audit->loginuid = loginuid; > > out: > > audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc); > > return rc; > > diff --git a/kernel/fork.c b/kernel/fork.c > > index 9440d61..1bfb0ff 100644 > > --- a/kernel/fork.c > > +++ b/kernel/fork.c > > @@ -1721,7 +1721,9 @@ static __latent_entropy struct task_struct *copy_process( > > p->start_time = ktime_get_ns(); > > p->real_start_time = ktime_get_boot_ns(); > > p->io_context = NULL; > > - audit_set_context(p, NULL); > > +#ifdef CONFIG_AUDITSYSCALL > > + p->audit = NULL; > > +#endif /* CONFIG_AUDITSYSCALL */ > > audit_alloc() is called a bit later and sets p->audit, so I don't think > this is really necessary. > > > cgroup_fork(p); > > #ifdef CONFIG_NUMA > > p->mempolicy = mpol_dup(p->mempolicy); - RGB -- Richard Guy Briggs Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635