On 10 August 2026 21:49:45 BST, Ricardo Robaina <[email protected]> wrote: >On Mon, Aug 10, 2026 at 1:28 PM Bradley Morgan <[email protected]> wrote: >> >> 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! >> > >It looks good to me. I've built a kernel to test it and verified that >it passes the audit testsuite. > ># uname -r >7.2.0-rc6+ > ># make test >make -C tests test >chmod +x */test >Running as user root > with context unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > on system Fedora > >amcast_joinpart/test ................. ok >backlog_wait_time_actual_reset/test .. ok >bpf/test ............................. ok >coredump/test ........................ ok >exec_execve/test ..................... ok >exec_name/test ....................... ok >fanotify/test ........................ ok >field_compare/test ................... ok >file_create/test ..................... ok >file_delete/test ..................... ok >file_permission/test ................. ok >file_rename/test ..................... ok >filter_device/test ................... ok >filter_exclude/test .................. ok >filter_exit/test ..................... ok >filter_inode/test .................... ok >filter_saddr_fam/test ................ ok >filter_sessionid/test ................ ok >io_uring/test ........................ ok >login_tty/test ....................... ok >lost_reset/test ...................... ok >netfilter_pkt/test ................... ok >signal/test .......................... ok >syscalls_file/test ................... ok >syscall_module/test .................. ok >syscall_socketcall/test .............. ok >time_change/test ..................... ok >user_msg/test ........................ ok >All tests successful. >Files=28, Tests=303, 46 wallclock secs ( 0.08 usr 0.03 sys + 14.50 >cusr 1.56 csys = 16.17 CPU) >Result: PASS > >Reviewed-by: Ricardo Robaina <[email protected]> >Tested-by: Ricardo Robaina <[email protected]> > >-Ricardo > >
woah, thanks for the review too. so do I like post or will Paul take it Thanks!
