Linux-audit Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: nhorman@tuxdriver.com, linux-api@vger.kernel.org,
	containers@lists.linux-foundation.org,
	LKML <linux-kernel@vger.kernel.org>,
	dhowells@redhat.com,
	Linux-Audit Mailing List <linux-audit@redhat.com>,
	netfilter-devel@vger.kernel.org, ebiederm@xmission.com,
	simo@redhat.com, netdev@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, Eric Paris <eparis@parisplace.org>,
	mpatel@redhat.com, Serge Hallyn <serge@hallyn.com>
Subject: Re: [PATCH ghak90 V9 01/13] audit: collect audit task parameters
Date: Sun, 5 Jul 2020 11:09:43 -0400
Message-ID: <CAHC9VhTx=4879F1MSXg4=Xd1i5rhEtyam6CakQhy=_ZjGtTaMA@mail.gmail.com> (raw)
In-Reply-To: <6abeb26e64489fc29b00c86b60b501c8b7316424.1593198710.git.rgb@redhat.com>

On Sat, Jun 27, 2020 at 9:21 AM Richard Guy Briggs <rgb@redhat.com> 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.
>
> Please see the upstream github issue
> https://github.com/linux-audit/audit-kernel/issues/81
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/audit.h | 49 +++++++++++++++++++++++------------
>  include/linux/sched.h |  7 +----
>  init/init_task.c      |  3 +--
>  init/main.c           |  2 ++
>  kernel/audit.c        | 71 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  kernel/audit.h        |  5 ++++
>  kernel/auditsc.c      | 26 ++++++++++---------
>  kernel/fork.c         |  1 -
>  8 files changed, 124 insertions(+), 40 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3fcd9ee49734..c2150415f9df 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -100,6 +100,16 @@ enum audit_nfcfgop {
>         AUDIT_XT_OP_UNREGISTER,
>  };
>
> +struct audit_task_info {
> +       kuid_t                  loginuid;
> +       unsigned int            sessionid;
> +#ifdef CONFIG_AUDITSYSCALL
> +       struct audit_context    *ctx;
> +#endif
> +};
> +
> +extern struct audit_task_info init_struct_audit;
> +
>  extern int is_audit_feature_set(int which);
>
>  extern int __init audit_register_class(int class, unsigned *list);

...

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b62e6aaf28f0..2213ac670386 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -34,7 +34,6 @@
>  #include <linux/kcsan.h>
>
>  /* task_struct member predeclarations (sorted alphabetically): */
> -struct audit_context;
>  struct backing_dev_info;
>  struct bio_list;
>  struct blk_plug;
> @@ -946,11 +945,7 @@ struct task_struct {
>         struct callback_head            *task_works;
>
>  #ifdef CONFIG_AUDIT
> -#ifdef CONFIG_AUDITSYSCALL
> -       struct audit_context            *audit_context;
> -#endif
> -       kuid_t                          loginuid;
> -       unsigned int                    sessionid;
> +       struct audit_task_info          *audit;
>  #endif
>         struct seccomp                  seccomp;

In the early days of this patchset we talked a lot about how to handle
the task_struct and the changes that would be necessary, ultimately
deciding that encapsulating all of the audit fields into an
audit_task_info struct.  However, what is puzzling me a bit at this
moment is why we are only including audit_task_info in task_info by
reference *and* making it a build time conditional (via CONFIG_AUDIT).

If audit is enabled at build time it would seem that we are always
going to allocate an audit_task_info struct, so I have to wonder why
we don't simply embed it inside the task_info struct (similar to the
seccomp struct in the snippet above?  Of course the audit_context
struct needs to remain as is, I'm talking only about the
task_info/audit_task_info struct.

Richard, I'm sure you can answer this off the top of your head, but
I'd have to go digging through the archives to pull out the relevant
discussions so I figured I would just ask you for a reminder ... ?  I
imagine it's also possible things have changed a bit since those early
discussions and the solution we arrived at then no longer makes as
much sense as it did before.

> diff --git a/init/init_task.c b/init/init_task.c
> index 15089d15010a..92d34c4b7702 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -130,8 +130,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_AUDIT
> -       .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 0ead83e86b5a..349470ad7458 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -96,6 +96,7 @@
>  #include <linux/jump_label.h>
>  #include <linux/mem_encrypt.h>
>  #include <linux/kcsan.h>
> +#include <linux/audit.h>
>
>  #include <asm/io.h>
>  #include <asm/bugs.h>
> @@ -1028,6 +1029,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/audit.c b/kernel/audit.c
> index 8c201f414226..5d8147a29291 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -203,6 +203,73 @@ struct audit_reply {
>         struct sk_buff *skb;
>  };
>
> +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 info block for a task
> + * @tsk: task
> + *
> + * Call audit_alloc_syscall to filter on the task information and
> + * allocate a per-task audit context if necessary.  This is called from
> + * copy_process, so no lock is needed.
> + */
> +int audit_alloc(struct task_struct *tsk)
> +{
> +       int ret = 0;
> +       struct audit_task_info *info;
> +
> +       info = kmem_cache_alloc(audit_task_cache, GFP_KERNEL);
> +       if (!info) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +       info->loginuid = audit_get_loginuid(current);
> +       info->sessionid = audit_get_sessionid(current);
> +       tsk->audit = info;
> +
> +       ret = audit_alloc_syscall(tsk);
> +       if (ret) {
> +               tsk->audit = NULL;
> +               kmem_cache_free(audit_task_cache, info);
> +       }
> +out:
> +       return ret;
> +}

This is a big nitpick, and I'm only mentioning this in the case you
need to respin this patchset: the "out" label is unnecessary in the
function above.  Simply return the error code, there is no need to
jump to "out" only to immediately return the error code there and
nothing more.

> +struct audit_task_info init_struct_audit = {
> +       .loginuid = INVALID_UID,
> +       .sessionid = AUDIT_SID_UNSET,
> +#ifdef CONFIG_AUDITSYSCALL
> +       .ctx = NULL,
> +#endif
> +};
> +
> +/**
> + * audit_free - free per-task audit info
> + * @tsk: task whose audit info block to free
> + *
> + * Called from copy_process and do_exit
> + */
> +void audit_free(struct task_struct *tsk)
> +{
> +       struct audit_task_info *info = tsk->audit;
> +
> +       audit_free_syscall(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);

Another nitpick, and this one may even become a moot point given the
question posed above.  However, is there any reason we couldn't get
rid of "info" and simplify this a bit?

  audit_free_syscall(tsk);
  kmem_cache_free(audit_task_cache, tsk->audit);
  tsk->audit = NULL;

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 468a23390457..f00c1da587ea 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk)
>                 if (context->current_state == AUDIT_RECORD_CONTEXT)
>                         audit_log_exit();
>         }
> -
>         audit_set_context(tsk, NULL);
>         audit_free_context(context);
>  }

This nitpick is barely worth the time it is taking me to write this,
but the whitespace change above isn't strictly necessary.


--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


  reply index

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-27 13:20 [PATCH ghak90 V9 00/13] audit: implement container identifier Richard Guy Briggs
2020-06-27 13:20 ` [PATCH ghak90 V9 01/13] audit: collect audit task parameters Richard Guy Briggs
2020-07-05 15:09   ` Paul Moore [this message]
2020-07-07  2:50     ` Richard Guy Briggs
2020-07-08  1:42       ` Paul Moore
2020-07-13 20:29         ` Richard Guy Briggs
2020-07-14  0:44           ` Paul Moore
2020-06-27 13:20 ` [PATCH ghak90 V9 02/13] audit: add container id Richard Guy Briggs
2020-07-04 13:29   ` Paul Moore
2020-07-04 13:30     ` Paul Moore
2020-07-05 15:09   ` Paul Moore
2020-07-29 20:05     ` Richard Guy Briggs
2020-08-21 19:36       ` Paul Moore
2020-06-27 13:20 ` [PATCH ghak90 V9 03/13] audit: read container ID of a process Richard Guy Briggs
2020-06-27 13:20 ` [PATCH ghak90 V9 04/13] audit: log drop of contid on exit of last task Richard Guy Briggs
2020-07-05 15:10   ` Paul Moore
2020-06-27 13:20 ` [PATCH ghak90 V9 05/13] audit: log container info of syscalls Richard Guy Briggs
2020-07-05 15:10   ` Paul Moore
2020-07-29 19:40     ` Richard Guy Briggs
2020-08-21 19:15       ` Paul Moore
2020-10-02 19:52         ` Richard Guy Briggs
2020-10-21 16:39           ` Richard Guy Briggs
2020-10-21 16:49             ` Steve Grubb
2020-10-21 17:53               ` Richard Guy Briggs
2020-10-23  1:21             ` Paul Moore
2020-10-23 20:40               ` Richard Guy Briggs
2020-10-28  1:35                 ` Paul Moore
2020-06-27 13:20 ` [PATCH ghak90 V9 06/13] audit: add contid support for signalling the audit daemon Richard Guy Briggs
2020-07-05 15:10   ` Paul Moore
2020-07-29 19:00     ` Richard Guy Briggs
2020-08-21 18:48       ` Paul Moore
2020-10-02 19:25         ` Richard Guy Briggs
2020-06-27 13:20 ` [PATCH ghak90 V9 07/13] audit: add support for non-syscall auxiliary records Richard Guy Briggs
2020-07-05 15:11   ` Paul Moore
2020-06-27 13:20 ` [PATCH ghak90 V9 08/13] audit: add containerid support for user records Richard Guy Briggs
2020-07-05 15:11   ` Paul Moore
2020-07-18  0:43     ` Richard Guy Briggs
2020-08-21 18:34       ` Paul Moore
2020-06-27 13:20 ` [PATCH ghak90 V9 09/13] audit: add containerid filtering Richard Guy Briggs
2020-06-27 13:20 ` [PATCH ghak90 V9 10/13] audit: add support for containerid to network namespaces Richard Guy Briggs
2020-07-05 15:11   ` Paul Moore
2020-07-21 22:05     ` Richard Guy Briggs
2020-06-27 13:20 ` [PATCH ghak90 V9 11/13] audit: contid check descendancy and nesting Richard Guy Briggs
2020-07-05 15:11   ` Paul Moore
2020-08-07 17:10     ` Richard Guy Briggs
2020-08-21 20:13       ` Paul Moore
2020-10-06 20:03         ` Richard Guy Briggs
2020-06-27 13:20 ` [PATCH ghak90 V9 12/13] audit: track container nesting Richard Guy Briggs
2020-07-05 15:11   ` Paul Moore
2020-06-27 13:20 ` [PATCH ghak90 V9 13/13] audit: add capcontid to set contid outside init_user_ns Richard Guy Briggs
2020-07-05 15:11   ` Paul Moore

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHC9VhTx=4879F1MSXg4=Xd1i5rhEtyam6CakQhy=_ZjGtTaMA@mail.gmail.com' \
    --to=paul@paul-moore.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=dhowells@redhat.com \
    --cc=ebiederm@xmission.com \
    --cc=eparis@parisplace.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mpatel@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=nhorman@tuxdriver.com \
    --cc=rgb@redhat.com \
    --cc=serge@hallyn.com \
    --cc=simo@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-audit Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-audit/0 linux-audit/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-audit linux-audit/ https://lore.kernel.org/linux-audit \
		linux-audit@redhat.com
	public-inbox-index linux-audit

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.linux-audit


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git