Re: [PATCH net] net: atm: fix use-after-free in sigd_put_skb()

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
net: atm: fix use-after-free in sigd_put_skb()

This commit addresses a use-after-free in sigd_put_skb() by pinning the
socket with find_get_vcc() and checking if it is marked SOCK_DEAD under
the callback lock.

> diff --git a/net/atm/signaling.c b/net/atm/signaling.c
> index b991d937205af..3dbe8e0fdc9af 100644
> --- a/net/atm/signaling.c
> +++ b/net/atm/signaling.c
> @@ -54,14 +54,31 @@ static struct atm_vcc *find_get_vcc(struct atm_vcc *vcc)
>  
>  static void sigd_put_skb(struct sk_buff *skb)
>  {
> -	if (!sigd) {
> +	struct atm_vcc *vcc;
> +	struct sock *sk;
> +
> +	vcc = find_get_vcc(READ_ONCE(sigd));

[Severity: Critical]
Does this introduce a regression where an ABA vulnerability allows a KASLR
bypass if the daemon socket is freed and reallocated?

If the signaling daemon is closed, sigd is cleared and the socket freed, but
an unprivileged thread in sigd_put_skb() might have already latched the old
sigd pointer.

If an unprivileged user subsequently creates a new ATM socket, couldn't the
slab allocator reuse the exact same memory address? In this case, find_get_vcc()
would match the raw pointer and return the attacker's new socket.

Since the attacker's socket isn't marked SOCK_DEAD, sigd_put_skb() would blindly
enqueue the signaling message to the attacker's receive queue.

Because the message contains literal kernel pointers (vcc, listen_vcc),
wouldn't this allow an unprivileged KASLR bypass and message injection?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does sigd_send() leak memory when queuing
SKBs to a closing listening socket?

When processing an as_indicate message, sigd_send() resolves the listening
socket via msg->listen_vcc, acquires lock_sock(), and appends an SKB:

net/atm/signaling.c:sigd_send() {
    ...
    lock_sock(sk);
    if (sk_acceptq_is_full(sk)) {
        ...
    }
    sk_acceptq_added(sk);
    skb_queue_tail(&sk->sk_receive_queue, skb);
    ...
}

Does this code blindly queue an SKB without checking if the socket has been
marked closed (ATM_VF_CLOSE) and already purged?

If the listening socket is concurrently closed, vcc_release() will lock the
socket, purge its receive queue, mark it closed, and release the lock.

Could sigd_send() then acquire the lock and append the new SKB to the dead
socket's receive queue? Since sk_free() expects an empty queue and does not
purge it during memory free, would this permanently leak the SKB and allow
an attacker to exhaust kernel memory?
-- 
This is an AI-generated review.