| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aafebe14727836ea747b97982926cc38@paul-moore.com>
Date: Tue, 05 Aug 2025 15:39:10 -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 v5 3/5] Audit: Add record for multiple task security contexts
On Jul 16, 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(-)
If there were no other issues with this patch I would have just fixed
this up during the merge (I did it in my review branch already), but
since you're no longer dependent on the LSM init rework changes (and
I've dropped the subj/obj counting in the latest revision), just go
ahead and base your next revision on the audit tree or Linus' tree as
one normally would.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 226c8ae00d04..c7dea6bfacdd 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
...
> +/**
> + * 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");
Argh ... please read prior review comments a bit more carefully. As was
pointed out in the v4 posting you're using the wrong function name here.
https://lore.kernel.org/audit/fc242f4c853fee16e587e9c78e1f282e@paul-moore.com
> + } 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");
... here is another case where you've missed/ignored previous feedback.
I believe this is the second revision in the history of this patchset
where you've missed feedback; *please* try to do better Casey, stuff like
this wastes time and drags things out longer than needed.
https://lore.kernel.org/audit/fc242f4c853fee16e587e9c78e1f282e@paul-moore.com
> audit_buffer_free(ab);
> }
--
paul-moore.com
Powered by blists - more mailing lists