[PATCH] net: qrtr: fix refcount saturation and potential UAF in qrtr_port_remove

[PATCH] net: qrtr: fix refcount saturation and potential UAF in qrtr_port_remove

[w15303746062]

From: Mingyu Wang <25181214217@stu.xidian.edu.cn>

In qrtr_port_remove(), the socket reference count is decremented via
__sock_put() before the port is removed from the qrtr_ports XArray and
before the RCU grace period elapses.

This breaks the fundamental RCU update paradigm. It exposes a race
window where a concurrent RCU reader (such as qrtr_reset_ports() or
qrtr_port_lookup()) can obtain a pointer to the socket from the XArray,
and attempt to call sock_hold() on a socket whose reference count has
already dropped to zero.

This exact race condition was hit during syzkaller fuzzing, leading to
the following refcount saturation warning and a potential Use-After-Free:

  refcount_t: saturated; leaking memory.
  WARNING: CPU: 3 PID: 1273 at lib/refcount.c:22 refcount_warn_saturate+0xae/0x1d0
  Modules linked in: qrtr(+) bochs drm_shmem_helper ...
  Call Trace:
   <TASK>
   qrtr_reset_ports net/qrtr/af_qrtr.c:768 [inline] [qrtr]
   __qrtr_bind.isra.0+0x48b/0x570 net/qrtr/af_qrtr.c:805 [qrtr]
   qrtr_bind+0x17d/0x210 net/qrtr/af_qrtr.c:901 [qrtr]
   kernel_bind+0xe4/0x120 net/socket.c:3592
   qrtr_ns_init+0x1a6/0x380 net/qrtr/ns.c:715 [qrtr]
   qrtr_proto_init+0x3b/0xff0 net/qrtr/af_qrtr.c:169 [qrtr]
   do_one_initcall+0xf5/0x5e0 init/main.c:1283
   ...
   </TASK>

Fix this by deferring the reference count decrement until after the
xa_erase() and the synchronize_rcu() complete. Furthermore, replace
__sock_put() with sock_put() to ensure that the socket memory is safely
freed if this happens to be the last reference, preventing memory leaks.

Fixes: a365023a76f2 ("net: qrtr: combine nameservice into main module")
Signed-off-by: Mingyu Wang <25181214217@stu.xidian.edu.cn>
---
 net/qrtr/af_qrtr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index 7cec6a7859b0..9ddc8657f7db 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -707,13 +707,13 @@ static void qrtr_port_remove(struct qrtr_sock *ipc)
 	if (port == QRTR_PORT_CTRL)
 		port = 0;
 
-	__sock_put(&ipc->sk);
-
 	xa_erase(&qrtr_ports, port);
 
 	/* Ensure that if qrtr_port_lookup() did enter the RCU read section we
 	 * wait for it to up increment the refcount */
 	synchronize_rcu();
+
+	sock_put(&ipc->sk);
 }
 
 /* Assign port number to socket.
-- 
2.34.1

Re: [PATCH] net: qrtr: fix refcount saturation and potential UAF in qrtr_port_remove

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

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
---
Subject: net: qrtr: fix refcount saturation and potential UAF in qrtr_port_remove

This patch defers the socket reference count decrement in
qrtr_port_remove() until after xa_erase() and synchronize_rcu() to
close a race with concurrent RCU readers, and also switches from
__sock_put() to sock_put().

> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index 7cec6a7859b0..9ddc8657f7db 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -707,13 +707,13 @@ static void qrtr_port_remove(struct qrtr_sock *ipc)
>  	if (port == QRTR_PORT_CTRL)
>  		port = 0;
>  
> -	__sock_put(&ipc->sk);
> -
>  	xa_erase(&qrtr_ports, port);
>  
>  	/* Ensure that if qrtr_port_lookup() did enter the RCU read section we
>  	 * wait for it to up increment the refcount */
>  	synchronize_rcu();
> +
> +	sock_put(&ipc->sk);
>  }

[Low]
Is the Fixes: tag pointing at the right commit?

> Fixes: a365023a76f2 ("net: qrtr: combine nameservice into main module")

Looking at a365023a76f2, that commit renames qrtr.c to af_qrtr.c and
consolidates the nameservice into the main module; it does not touch the
__sock_put()/xa_erase()/synchronize_rcu() sequence in qrtr_port_remove().

A history search with git log -S '__sock_put(&ipc->sk)' on net/qrtr/
shows this pattern was first introduced by:

  bdabad3e363d ("net: Add Qualcomm IPC router")

Should the Fixes: tag reference bdabad3e363d so stable picks up the fix
on every release in the bdabad3e363d..a365023a76f2 range?

[Low]
Is the rationale given for switching __sock_put() to sock_put() accurate?

The commit message states:

  Furthermore, replace __sock_put() with sock_put() to ensure that the
  socket memory is safely freed if this happens to be the last
  reference, preventing memory leaks.

Can the xarray reference legitimately be the last one at this point?

The two callers of qrtr_port_remove() both keep additional references
alive across the call.

In qrtr_release(), sk is still dereferenced after qrtr_port_remove()
returns:

	if (!sock_flag(sk, SOCK_ZAPPED))
		qrtr_port_remove(ipc);

	skb_queue_purge(&sk->sk_receive_queue);

	release_sock(sk);
	sock_put(sk);

In __qrtr_bind(), the rebind path calls qrtr_port_assign() (which does
sock_hold() for the new port) before qrtr_port_remove():

	rc = qrtr_port_assign(ipc, &port);
	if (rc)
		return rc;

	/* unbind previous, if any */
	if (!zapped)
		qrtr_port_remove(ipc);

If sock_put() ever did free the socket here, qrtr_release()'s subsequent
skb_queue_purge(&sk->sk_receive_queue) and release_sock(sk) would be a
use-after-free, which would indicate a separate refcount-imbalance bug
rather than a memory leak.

Could the commit message be reworded so it doesn't conflate the
__sock_put() to sock_put() change with a memory leak fix?

Simon says: I would go a bit further here. Based on the above
            I think the __sock_put() -> sock_put() should be dropped.