Re: [PATCH] audit: use copied skb length in kauditd_send_multicast_skb()

[Bradley Morgan]

On 10 August 2026 21:49:45 BST, Ricardo Robaina <rrobaina@xxxxxxxxxx>
wrote:
>On Mon, Aug 10, 2026 at 1:28 PM Bradley Morgan <include@xxxxxxxxx> wrote:
>>
>> On 10 August 2026 16:19:10 BST, Ricardo Robaina <rrobaina@xxxxxxxxxx>
>> wrote:
>> >On Mon, Aug 10, 2026 at 10:32 AM Bradley Morgan <include@xxxxxxxxx>
>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 <include@xxxxxxxxx>
>> 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 <include@xxxxxxxxx>
>> ---
>>  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 <include@xxxxxxxxx>
>> >> 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 <rrobaina@xxxxxxxxxx>
>Tested-by: Ricardo Robaina <rrobaina@xxxxxxxxxx>
>
>-Ricardo
>
>

woah, thanks for the review too. so do I like post or will Paul take it

Thanks!