Message-ID: <fc242f4c853fee16e587e9c78e1f282e@paul-moore.com>
Date: Mon, 16 Jun 2025 16:54:51 -0400
From: Paul Moore <paul@...l-moore.com>
To: Casey Schaufler <casey@...aufler-ca.com>, casey@...aufler-ca.com, eparis@...hat.com, linux-security-module@...r.kernel.org, audit@...r.kernel.org
Cc: jmorris@...ei.org, serge@...lyn.com, keescook@...omium.org, john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp, stephen.smalley.work@...il.com, linux-kernel@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [PATCH v4 3/4] Audit: Add record for multiple task security  contexts

On Jun  6, 2025 Casey Schaufler <casey@...aufler-ca.com> wrote:
> 
> Replace the single skb pointer in an audit_buffer with a list of
> skb pointers. Add the audit_stamp information to the audit_buffer as
> there's no guarantee that there will be an audit_context containing
> the stamp associated with the event. At audit_log_end() time create
> auxiliary records as have been added to the list. Functions are
> created to manage the skb list in the audit_buffer.
> 
> Create a new audit record AUDIT_MAC_TASK_CONTEXTS.
> An example of the MAC_TASK_CONTEXTS record is:
> 
>     type=MAC_TASK_CONTEXTS
>     msg=audit(1600880931.832:113)
>     subj_apparmor=unconfined
>     subj_smack=_
> 
> When an audit event includes a AUDIT_MAC_TASK_CONTEXTS record the
> "subj=" field in other records in the event will be "subj=?".
> An AUDIT_MAC_TASK_CONTEXTS record is supplied when the system has
> multiple security modules that may make access decisions based on a
> subject security context.
> 
> Refactor audit_log_task_context(), creating a new audit_log_subj_ctx().
> This is used in netlabel auditing to provide multiple subject security
> contexts as necessary.
> 
> Suggested-by: Paul Moore <paul@...l-moore.com>
> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
> ---
>  include/linux/audit.h        |  16 +++
>  include/uapi/linux/audit.h   |   1 +
>  kernel/audit.c               | 207 +++++++++++++++++++++++++++++------
>  net/netlabel/netlabel_user.c |   9 +-
>  security/apparmor/lsm.c      |   3 +
>  security/lsm.h               |   4 -
>  security/lsm_init.c          |   5 -
>  security/security.c          |   3 -
>  security/selinux/hooks.c     |   3 +
>  security/smack/smack_lsm.c   |   3 +
>  10 files changed, 202 insertions(+), 52 deletions(-)

...

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 0050ef288ab3..5020939fb8bc 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -37,6 +37,8 @@ struct audit_watch;
>  struct audit_tree;
>  struct sk_buff;
>  struct kern_ipc_perm;
> +struct lsm_id;
> +struct lsm_prop;
>  
>  struct audit_krule {
>  	u32			pflags;
> @@ -147,6 +149,9 @@ extern unsigned compat_signal_class[];
>  #define AUDIT_TTY_ENABLE	BIT(0)
>  #define AUDIT_TTY_LOG_PASSWD	BIT(1)
>  
> +/* bit values for audit_lsm_secctx */
> +#define AUDIT_SECCTX_SUBJECT	BIT(0)

More on naming below, but how about AUDIT_CFG_LSM_SECCTX_SUBJ?

> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 9a4ecc9f6dc5..8cad2f307719 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -148,6 +148,7 @@
>  #define AUDIT_IPE_POLICY_LOAD	1422	/* IPE policy load */
>  #define AUDIT_LANDLOCK_ACCESS	1423	/* Landlock denial */
>  #define AUDIT_LANDLOCK_DOMAIN	1424	/* Landlock domain status */
> +#define AUDIT_MAC_TASK_CONTEXTS	1425	/* Multiple LSM task contexts */
>  
>  #define AUDIT_FIRST_KERN_ANOM_MSG   1700
>  #define AUDIT_LAST_KERN_ANOM_MSG    1799
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 6bbadb605ca3..0987b2f391cc 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -278,6 +286,27 @@ static pid_t auditd_pid_vnr(void)
>  	return pid;
>  }
>  
> +/**
> + * audit_lsm_secctx - Identify a security module as providing a secctx.
> + * @lsmid: LSM identity
> + * @flags: which contexts are provided
> + *
> + * Description:
> + * Increments the count of the security modules providing a secctx.
> + * If the LSM id is already in the list leave it alone.
> + */
> +void audit_lsm_secctx(const struct lsm_id *lsmid, int flags)
> +{
> +	int i;
> +
> +	if (flags & AUDIT_SECCTX_SUBJECT) {
> +		for (i = 0 ; i < audit_subj_secctx_cnt; i++)
> +			if (audit_subj_lsms[i] == lsmid)
> +				return;
> +		audit_subj_lsms[audit_subj_secctx_cnt++] = lsmid;
> +	}
> +}

Naming is hard ... but since this function has nothing to do with logging
the current state of execution, and is instead more about signaling and
configuring audit, let's try and make that a bit more clear in the
function name while also not getting us stuck with just a secctx config.

How about "audit_cfg_lsm(...)"?

> +/**
> + * audit_log_subj_ctx - Add LSM subject information
> + * @ab: audit_buffer
> + * @prop: LSM subject properties.
> + *
> + * Add a subj= field and, if necessary, a AUDIT_MAC_TASK_CONTEXTS record.
> + */
> +int audit_log_subj_ctx(struct audit_buffer *ab, struct lsm_prop *prop)
>  {
> -	struct lsm_prop prop;
>  	struct lsm_context ctx;
> +	char *space = "";
>  	int error;
> +	int i;
>  
> -	security_current_getlsmprop_subj(&prop);
> -	if (!lsmprop_is_set(&prop))
> +	security_current_getlsmprop_subj(prop);
> +	if (!lsmprop_is_set(prop))
>  		return 0;
>  
> -	error = security_lsmprop_to_secctx(&prop, &ctx, LSM_ID_UNDEF);
> -	if (error < 0) {
> -		if (error != -EINVAL)
> -			goto error_path;
> +	if (audit_subj_secctx_cnt < 2) {
> +		error = security_lsmprop_to_secctx(prop, &ctx, LSM_ID_UNDEF);
> +		if (error < 0) {
> +			if (error != -EINVAL)
> +				goto error_path;
> +			return 0;
> +		}
> +		audit_log_format(ab, " subj=%s", ctx.context);
> +		security_release_secctx(&ctx);
>  		return 0;
>  	}
> -
> -	audit_log_format(ab, " subj=%s", ctx.context);
> -	security_release_secctx(&ctx);
> +	/* Multiple LSMs provide contexts. Include an aux record. */
> +	audit_log_format(ab, " subj=?");
> +	error = audit_buffer_aux_new(ab, AUDIT_MAC_TASK_CONTEXTS);
> +	if (error)
> +		goto error_path;
> +
> +	for (i = 0; i < audit_subj_secctx_cnt; i++) {
> +		error = security_lsmprop_to_secctx(prop, &ctx,
> +						   audit_subj_lsms[i]->id);
> +		if (error < 0) {
> +			/*
> +			 * Don't print anything. An LSM like BPF could
> +			 * claim to support contexts, but only do so under
> +			 * certain conditions.
> +			 */
> +			if (error == -EOPNOTSUPP)
> +				continue;
> +			if (error != -EINVAL)
> +				audit_panic("error in audit_log_task_context");

Wrong function name, maybe use __func__ or something similar.

> +		} else {
> +			audit_log_format(ab, "%ssubj_%s=%s", space,
> +					 audit_subj_lsms[i]->name, ctx.context);
> +			space = " ";
> +			security_release_secctx(&ctx);
> +		}
> +	}
> +	audit_buffer_aux_end(ab);
>  	return 0;
>  
>  error_path:
> -	audit_panic("error in audit_log_task_context");
> +	audit_panic("error in audit_log_subj_ctx");
>  	return error;
>  }
> +EXPORT_SYMBOL(audit_log_subj_ctx);

...

> @@ -2423,25 +2575,16 @@ int audit_signal_info(int sig, struct task_struct *t)
>  void audit_log_end(struct audit_buffer *ab)
>  {
>  	struct sk_buff *skb;
> -	struct nlmsghdr *nlh;
>  
>  	if (!ab)
>  		return;
>  
> -	if (audit_rate_check()) {
> -		skb = ab->skb;
> -		ab->skb = NULL;
> +	while ((skb = skb_dequeue(&ab->skb_list)))
> +		__audit_log_end(skb);
>  
> -		/* setup the netlink header, see the comments in
> -		 * kauditd_send_multicast_skb() for length quirks */
> -		nlh = nlmsg_hdr(skb);
> -		nlh->nlmsg_len = skb->len - NLMSG_HDRLEN;
> -
> -		/* queue the netlink packet and poke the kauditd thread */
> -		skb_queue_tail(&audit_queue, skb);
> +	/* poke the kauditd thread */
> +	if (audit_rate_check())
>  		wake_up_interruptible(&kauditd_wait);
> -	} else
> -		audit_log_lost("rate limit exceeded");
>  
>  	audit_buffer_free(ab);
>  }

Don't make wake_up_interruptible() dependent on audit_rate_check(),
wake up the kauditd thread regardless.  Yes, arguably there will be
some cases where the wake will likely be unnecessary, but
audit_rate_check() exists to limit the number of records emitted by
the kernel which is being handled in __audit_log_end(); we don't want
to limit waking the kauditd thread.  There is also the issue of the
spin lock which we should try to minimize.

If you're really bothered by calling wake_up_interruptible() without
taking into account if any records have been queued, you could always
have __audit_log_end() return a count/flag/etc. to indicate if any
records have been queued for kauditd, but I suspect that's more
trouble than it's worth.

--
paul-moore.com

Powered by blists - more mailing lists