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
