Re: [PATCH] audit: fix UAF in kauditd_send_queue()

[Bradley Morgan <brads@mainlining.org>]

On 31 October 2026 21:12:59 GMT, "Jérémy Jean"
<Jeremy.Jean@oss.cyber.gouv.fr> wrote:
>kauditd_send_queue() grabs an extra reference to the skb header before
>netlink_unicast() since it may need to retry a failed send. If unicast
>delivery reuses that same header, the receive path stores the
>destination socket in skb->sk without taking a socket reference.
>Consequently, after the receiver consumes the skb and closes the
>socket, the extra reference taken on the skb header makes kauditd
>release a header whose skb->sk points to freed socket storage.
>
>KASAN reports a UAF as:
>
>  BUG: KASAN: slab-use-after-free in sock_rfree+0x44/0x140
>  Write of size 4 at addr ffff888004f94140 by task kauditd/31
>
>Clone the header before netlink_unicast(), then use the clone for
>delivery and keep the original skb only for retries. If cloning fails,
>handle it as -EAGAIN so the send can be retried.
>
>Fixes: f3d357b09295 ("Audit: save audit_backlog_limit audit messages in case auditd comes back")
>Cc: stable@vger.kernel.org

What I cant work out from the changelog is where netlink_trim() went.
With users == 2 it usually clones for you anyway (skb_shared() is just
users != 1) and then the receiver owns a different struct, so this
couldnt happen. Unless it took the early return:

	if (is_vmalloc_addr(skb->head) || delta * 2 < skb->truesize)
		return skb;

Which one was it? If its the delta side then its the tightly filled
records that trip this, which explains 18 years of silence pretty well.
It also tells us what this costs, where trim would have cloned anyway
you just moved the clone, otherwise you pay one small slab alloc per
record.

>Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
>---
> kernel/audit.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
>diff --git a/kernel/audit.c b/kernel/audit.c
>index 9412af9144bc..5e739fa606be 100644
>--- a/kernel/audit.c
>+++ b/kernel/audit.c
>@@ -790,6 +790,7 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
> {
> 	int rc = 0;
> 	struct sk_buff *skb = NULL;
>+	struct sk_buff *skb_to_send;
> 	struct sk_buff *skb_tail;
> 	unsigned int failed = 0;
> 
>@@ -810,9 +811,11 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
> 		}
> 
> retry:
>-		/* grab an extra skb reference in case of error */
>-		skb_get(skb);
>-		rc = netlink_unicast(sk, skb, portid, 0);
>+		/* Keep the original header unowned for retries. */


A header isnt a skb, and the comment you deleted said skb. Also this
line is where the why belongs, since the why is the bug itself. e.g,

		/* netlink remembers the sock in skb->sk with no ref,
		 * so send a clone and keep the original clean
		 */


>+		skb_to_send = skb_clone(skb, GFP_KERNEL);
>+		/* Preserve the connection on a temporary allocation failure. */

Well, this one doesnt preserve the connection.. -EAGAIN falls into the
same bail branch which does sk = NULL and then squashes rc to 0, so
once the alloc failures hit retry_limit the connection is gone anyway.

It also eats a slot of ++failed for what is really just an ENOMEM and
not a send failure. Might be fine! but the comment says the opposite of
the code, can you make it match?

>+		rc = skb_to_send ?
>+			netlink_unicast(sk, skb_to_send, portid, 0) : -EAGAIN;
> 		if (rc < 0) {
> 			/* send failed - try a few times unless fatal error */
> 			if (++failed >= retry_limit ||
>@@ -827,7 +830,7 @@ retry:
> 			} else
> 				goto retry;
> 		} else {
>-			/* skb sent - drop the extra reference and continue */
>+			/* skb sent - consume the original and continue */
> 			consume_skb(skb);
> 			failed = 0;
> 		}
>

--- Thanks!