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

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

[Weiming Shi]

sigd_put_skb() delivers a signalling message to the daemon socket named
by the global @sigd pointer, ending in a call to sk_data_ready(). It
reads @sigd with no synchronisation, so it can race with a close of the
daemon socket: sigd_close() clears @sigd and the socket is then torn
down and freed.

Holding a reference on the socket is not enough to make this safe. The
daemon fd close runs __sock_release(), which frees the struct socket --
and the wait queue that sk->sk_wq points at -- via iput() once
->release() has returned. sk_data_ready() (sock_def_readable()) then
dereferences a freed sk_wq:

  Oops: general protection fault, probably for non-canonical address 0xdffffc0000000031: 0000 [#1] SMP KASAN NOPTI
  KASAN: null-ptr-deref in range [0x0000000000000188-0x000000000000018f]
  RIP: 0010:sigd_put_skb (net/atm/signaling.c:65)
   sigd_enq2 (net/atm/signaling.c:228)
   sigd_enq (net/atm/signaling.c:237)
   svc_bind (net/atm/svc.c:135)
   __sys_bind
   __x64_sys_bind
   do_syscall_64

Fix it on both sides. sigd_close() now calls sock_orphan(), which under
sk_callback_lock sets SOCK_DEAD and clears sk_wq before the socket is
freed. sigd_put_skb() latches @sigd with READ_ONCE(), pins the socket
with find_get_vcc(), and then takes sk_callback_lock; if the socket is
already SOCK_DEAD it drops the skb, otherwise it delivers while the lock
keeps sk_wq valid. sk_callback_lock is used rather than lock_sock()
because sigd_put_skb() can be reached from vcc_sendmsg() -> sigd_send(),
which already holds lock_sock() on the daemon socket.

Triggering the race requires CAP_NET_ADMIN and CAP_SYS_RAWIO to attach
the daemon.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Xiang Mei <xmei5@asu.edu>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 net/atm/signaling.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/net/atm/signaling.c b/net/atm/signaling.c
index b991d937205a..3dbe8e0fdc9a 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));
+	if (!vcc) {
 		pr_debug("atmsvc: no signaling daemon\n");
 		kfree_skb(skb);
 		return;
 	}
-	atm_force_charge(sigd, skb->truesize);
-	skb_queue_tail(&sk_atm(sigd)->sk_receive_queue, skb);
-	sk_atm(sigd)->sk_data_ready(sk_atm(sigd));
+	sk = sk_atm(vcc);
+
+	/* Pairs with sock_orphan() in sigd_close(). */
+	read_lock_bh(&sk->sk_callback_lock);
+	if (sock_flag(sk, SOCK_DEAD)) {
+		read_unlock_bh(&sk->sk_callback_lock);
+		sock_put(sk);
+		kfree_skb(skb);
+		return;
+	}
+	atm_force_charge(vcc, skb->truesize);
+	skb_queue_tail(&sk->sk_receive_queue, skb);
+	sk->sk_data_ready(sk);
+	read_unlock_bh(&sk->sk_callback_lock);
+
+	sock_put(sk);
 }
 
 static void modify_qos(struct atm_vcc *vcc, struct atmsvc_msg *msg)
@@ -258,6 +275,9 @@ static void sigd_close(struct atm_vcc *vcc)
 		pr_err("closing with requests pending\n");
 	skb_queue_purge(&sk_atm(vcc)->sk_receive_queue);
 
+	/* Make a concurrent sigd_put_skb() observe SOCK_DEAD and bail. */
+	sock_orphan(sk_atm(vcc));
+
 	read_lock(&vcc_sklist_lock);
 	for (i = 0; i < VCC_HTABLE_SIZE; ++i) {
 		struct hlist_head *head = &vcc_hash[i];
-- 
2.43.0

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

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

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.

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@example.com>
> 
> 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.