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

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