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

[Weiming Shi]

On 26-06-09 13:06, Paolo Abeni wrote:
> 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.
> 

Thanks. The find_get_vcc() issue will be fixed in v2; the sigd_send()
leak will be sent as a separate patch.