[PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time

[PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time

[Bradley Morgan]

Right now the auditd breakage (nlmsg_len gets set to the payload
length instead of the full message length) is applied when the record
is queued, in __audit_log_end(). That is why
kauditd_send_multicast_skb() has to deep copy every record and then
undo the length on the copy, just so the multicast group still sees a
standard netlink message.

So flip it: finalize the header with the standard full length at
queue time, and apply the auditd length at send time in
kauditd_send_queue(), right before the unicast. Records stay standard
netlink messages the whole time they sit in the queues, and the
multicast copy stops needing its own fixup. The copy itself stays,
because the rewrite still lands in the data region the listeners
already hold.

auditd sees the same bytes as before: the fixup is computed from
skb->len and that does not change between queueing and sending, so
records that come back around through the retry and hold queues get
the same value again. Reply and rule list skbs are built with
nlmsg_put() and go out on their own paths, none of that is touched.

This came out of reviewing Ricardo's "use copied skb length" patch,
where I suggested moving the fixup as the more interesting cleanup.

Reviewed-by: Ricardo Robaina <rrobaina@redhat.com>
Tested-by: Ricardo Robaina <rrobaina@redhat.com>
Signed-off-by: Bradley Morgan <include@grrlz.net>
Link: https://lore.kernel.org/r/20260810125726.775689-2-rrobaina@redhat.com
---
 kernel/audit.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 9412af9144bc..bcfed6e3678e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -802,6 +802,12 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
 		if (skb_hook)
 			(*skb_hook)(skb);
 
+		/*
+		 * auditd wants nlmsg_len to be the payload length, not the
+		 * full length, so break it here at send time.
+		 */
+		nlmsg_hdr(skb)->nlmsg_len = skb->len - NLMSG_HDRLEN;
+
 		/* can we send to anyone via unicast? */
 		if (!sk) {
 			if (err_hook)
@@ -849,7 +855,6 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb)
 {
 	struct sk_buff *copy;
 	struct sock *sock = audit_get_sk(&init_net);
-	struct nlmsghdr *nlh;
 
 	/* NOTE: we are not taking an additional reference for init_net since
 	 *       we don't have to worry about it going away */
@@ -858,20 +863,12 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb)
 		return;
 
 	/*
-	 * The seemingly wasteful skb_copy() rather than bumping the refcount
-	 * using skb_get() is necessary because non-standard mods are made to
-	 * the skb by the original kaudit unicast socket send routine.  The
-	 * existing auditd daemon assumes this breakage.  Fixing this would
-	 * require co-ordinating a change in the established protocol between
-	 * the kaudit kernel subsystem and the auditd userspace code.  There is
-	 * no reason for new multicast clients to continue with this
-	 * non-compliance.
+	 * skb_copy() rather than skb_get(): kauditd_send_queue() breaks
+	 * nlmsg_len for auditd, keep the listeners on a standard message.
 	 */
 	copy = skb_copy(skb, GFP_KERNEL);
 	if (!copy)
 		return;
-	nlh = nlmsg_hdr(copy);
-	nlh->nlmsg_len = skb->len;
 
 	nlmsg_multicast(sock, copy, 0, AUDIT_NLGRP_READLOG, GFP_KERNEL);
 }
@@ -2785,13 +2782,12 @@ int audit_signal_info(int sig, struct task_struct *t)
  */
 static void __audit_log_end(struct sk_buff *skb)
 {
-	struct nlmsghdr *nlh;
-
 	if (audit_rate_check()) {
-		/* 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;
+		/*
+		 * Records are built without keeping nlmsg_len up to date,
+		 * finalize it here with the full message length.
+		 */
+		nlmsg_end(skb, nlmsg_hdr(skb));
 
 		/* queue the netlink packet */
 		skb_queue_tail(&audit_queue, skb);
-- 
2.47.3

Re: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time

[sashiko-bot]

> Right now the auditd breakage (nlmsg_len gets set to the payload
> length instead of the full message length) is applied when the record
> is queued, in __audit_log_end(). That is why
> kauditd_send_multicast_skb() has to deep copy every record and then
> undo the length on the copy, just so the multicast group still sees a
> standard netlink message.
> 
> So flip it: finalize the header with the standard full length at
> queue time, and apply the auditd length at send time in
> kauditd_send_queue(), right before the unicast. Records stay standard
> netlink messages the whole time they sit in the queues, and the
> multicast copy stops needing its own fixup. The copy itself stays,
> because the rewrite still lands in the data region the listeners
> already hold.
> 
> auditd sees the same bytes as before: the fixup is computed from
> skb->len and that does not change between queueing and sending, so
> records that come back around through the retry and hold queues get
> the same value again. Reply and rule list skbs are built with
> nlmsg_put() and go out on their own paths, none of that is touched.
> 
> This came out of reviewing Ricardo's "use copied skb length" patch,
> where I suggested moving the fixup as the more interesting cleanup.
> 
> Reviewed-by: Ricardo Robaina <rrobaina@redhat.com>
> Tested-by: Ricardo Robaina <rrobaina@redhat.com>
> Signed-off-by: Bradley Morgan <include@grrlz.net>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814010153.14488-1-include@grrlz.net?part=1