On 10 August 2026 16:19:10 BST, Ricardo Robaina <[email protected]> wrote: >On Mon, Aug 10, 2026 at 10:32 AM Bradley Morgan <[email protected]> wrote: >> >> Hi Ricardo, >> >> > - nlh->nlmsg_len = skb->len; >> > + nlh->nlmsg_len = copy->len; >> >> Fine. skb_copy() does skb_put(n, skb->len) and nothing touches >> copy->len in between, so the two are always identical. >> Which is also why "safer" is a bit of a stretch, it prevents nothing. >> Feel free to bikeshed the changelog wording on that. >> > >Thanks for reviewing this patch, Bradley. I agree with you on the wording. >
np. >> If you ever want a more interesting cleanup here, the real quirk is >> the line above: the unicast path sets nlmsg_len to skb->len minus >> NLMSG_HDRLEN, this one uses the full length. > >That's interesting, maybe moving the nlmsg_len fixup out of >__audit_log_end() would be better. I'll look into it. > Hmmm.. perhaps it will. I will suggest a fix: >From 776375d625cd9cb138f7a6f876861bf428ae1e6c Mon Sep 17 00:00:00 2001 From: Bradley Morgan <[email protected]> Date: Mon, 10 Aug 2026 16:26:06 +0000 Subject: [PATCH] audit: move the nlmsg_len fixup from __audit_log_end() to send time The auditd breakage (nlmsg_len set to the payload length instead of the full message length) is applied in __audit_log_end() when the record is queued. That forces kauditd_send_multicast_skb() to deep copy every record and undo the length on the copy, so the multicast group still sees a standard netlink message. Instead, finalize the header with the standard full length at queue time and apply the auditd length right before the unicast send in kauditd_send_queue(). Records stay standard netlink messages while they sit in the queues, and the multicast copy no longer needs its own fixup. The deep copy itself stays, since the auditd length rewrite lands in the shared data region after the copy is already handed to the listeners. auditd sees the same bytes as before. The fixup is computed from skb->len, which does not change between queueing and sending, so records that come back through the retry and hold queues get the same value again. Reply and rule list skbs are built with nlmsg_put() and sent on other paths, none of those are touched. This came out of the "use copied skb length" thread, where moving the fixup was suggested as the more interesting cleanup. Signed-off-by: Bradley Morgan <[email protected]> --- kernel/audit.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index 9412af9144bc..5b6528fc6eb5 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -802,6 +802,15 @@ static int kauditd_send_queue(struct sock *sk, u32 portid, if (skb_hook) (*skb_hook)(skb); + /* + * auditd expects nlmsg_len to be the payload length rather + * than the full message length. Apply the breakage here at + * send time so the record stays a standard netlink message + * while queued and while it is copied for the multicast + * group above. + */ + nlmsg_hdr(skb)->nlmsg_len = skb->len - NLMSG_HDRLEN; + /* can we send to anyone via unicast? */ if (!sk) { if (err_hook) @@ -849,7 +858,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 */ @@ -859,19 +867,15 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb) /* * 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. + * using skb_get() is necessary because the unicast send in + * kauditd_send_queue() rewrites nlmsg_len to the payload only length + * that auditd expects. The copy shields the multicast listeners from + * that historical breakage, there is no reason for them to continue + * with this non compliance. */ 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 +2789,15 @@ 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; + /* + * The record was built by appending data after nlmsg_put() + * without keeping nlmsg_len up to date, so finalize the + * header here with the standard full message length. The + * payload only length that auditd expects is applied at + * send time in kauditd_send_queue(). + */ + nlmsg_end(skb, nlmsg_hdr(skb)); /* queue the netlink packet */ skb_queue_tail(&audit_queue, skb); -- 2.47.3 >> >> Well, why not, please add: >> >> Reviewed-by: Bradley Morgan <[email protected]> >> Thanks! >> > >-Ricardo > > Thanks!
