[PATCH v3] RDMA/rxe: Fix use-after-free of the per-net tunnel socket

[PATCH v3] RDMA/rxe: Fix use-after-free of the per-net tunnel socket

[Binbin Deng]

KASAN reported a slab-use-after-free in ip6_route_output_flags()
reached from rxe_find_route(): a user with CAP_NET_ADMIN can remove
an RXE device with "rdma link del rxe0" while RoCE v2 over IPv6
traffic keeps using the per-net tunnel socket for route lookups.

BUG: KASAN: slab-use-after-free in ip6_route_output_flags+0x300/0x360
Read of size 4 at addr ffff888115ddd794 by task kworker/u32:6/309
Call Trace:
 <TASK>
 dump_stack_lvl+0x53/0x70
 print_report+0xd0/0x630
 ? __pfx__raw_spin_lock_irqsave+0x10/0x10
 ? ip6_route_output_flags+0x300/0x360
 kasan_report+0xce/0x100
 ? ip6_route_output_flags+0x300/0x360
 ip6_route_output_flags+0x300/0x360
 ip6_dst_lookup_tail.constprop.0+0x76c/0xcc0
 ? ct_nmi_exit+0xc3/0xf0
 ip6_dst_lookup_flow+0xf5/0x1e0
 ? __pfx_ip6_dst_lookup_flow+0x10/0x10
 rxe_find_route+0x426/0xa30
 ? __kasan_slab_alloc+0x6e/0x70
 ? __pfx_rxe_find_route+0x10/0x10
 ? kmem_cache_alloc_node_noprof+0x141/0x370
 ? kmalloc_reserve+0x103/0x2b0
 ? rxe_icrc_generate+0x229/0x330
 ? __pfx___alloc_skb+0x10/0x10
 rxe_prepare+0x9e8/0x18b0
 ? rxe_init_packet+0x3c7/0x4f0
 rxe_requester+0x1a0f/0x51f0
 ? rxe_completer+0x1de9/0x38c0
 ? __pfx_rxe_completer+0x10/0x10
 ? __queue_work+0x43e/0x11f0
 ? __pfx_rxe_requester+0x10/0x10
 ? irqentry_exit+0xd2/0x640
 ? _raw_spin_lock_irqsave+0x85/0xe0
 ? __pfx__raw_spin_lock_irqsave+0x10/0x10
 ? __pfx_rxe_sender+0x10/0x10
 rxe_sender+0xe/0x30
 do_work+0x144/0x470
 process_one_work+0x633/0x1030
 ? assign_work+0x11d/0x370
 worker_thread+0x45b/0xd10
 ? __pfx_worker_thread+0x10/0x10
 kthread+0x2c6/0x3b0
 ? recalc_sigpending+0x15c/0x1e0
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x36e/0x5a0
 ? __pfx_ret_from_fork+0x10/0x10
 ? __switch_to+0x572/0xdd0
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1a/0x30
 </TASK>

Allocated by task 146020:
 kasan_save_stack+0x33/0x60
 kasan_save_track+0x14/0x30
 __kasan_slab_alloc+0x6e/0x70
 kmem_cache_alloc_noprof+0x130/0x360
 sk_prot_alloc+0x56/0x210

rxe_net_del() releases the tunnel socket before the device is torn
down: nldev_dellink() calls .dellink before unregistering the
device, and the NETDEV_UNREGISTER notifier calls rxe_net_del() right
after queueing the unregistration, so the rxe_sender() workqueue can
still perform route lookups while the socket is released.

Two gaps in the tunnel socket lifetime allow the use-after-free:

1. rxe_sock_put() released the socket with rxe_release_udp_tunnel()
   before the per-net pointer was cleared, so route lookups could
   fetch a socket that was already freed.

2. rxe_ns_pernet_sk4/6() return the raw socket pointer after
   rcu_read_unlock(), without taking a reference.  Readers like
   rxe_find_route6() dereference the pointer outside the RCU
   read-side critical section, so the synchronize_rcu() in
   rxe_ns_pernet_set_sk4/6() cannot protect a reader that already
   fetched the pointer, and ip6_route_output_flags() or
   xfrm_lookup_with_ifid() can still be using the socket while it
   is freed.

Fix both gaps:

- rxe_sock_put() clears the per-net pointer before it releases the
  socket, so no new lookup can fetch a socket that is being
  released;

- rxe_ns_pernet_sk4/6() take a reference on the socket with
  refcount_inc_not_zero() under rcu_read_lock(), and the callers
  sock_put() it after use.  The reference that rxe_net_ipv4/6_init()
  keeps now comes from the getter instead of an open-coded
  sock_hold() on the raw pointer, and rxe_sock_put() drops the
  getter's reference after the tunnel is released.  Since the
  reference is taken inside the RCU read-side critical section, the
  synchronize_rcu() after the pointer is cleared guarantees that
  every reader has finished taking its reference before the last
  reference is dropped, so the count never reaches zero while a
  reader still holds the pointer.

The remaining races on the tunnel socket lifetime (serialization of
rxe_net_init() against rxe_net_del(), a second rxe_net_del() of one
device from the NETDEV_UNREGISTER notifier, and the per-device
reference accounting of shared sockets) already exist in the code
this fixes and are left for a follow-up series.

Fixes: f1327abd6abe ("RDMA/rxe: Support RDMA link creation and destruction per net namespace")
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 drivers/infiniband/sw/rxe/rxe_net.c | 29 ++++++++++++++++++++---------
 drivers/infiniband/sw/rxe/rxe_ns.c  | 10 ++++++++++
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index 53daaf4c1eb2..ec98133a6005 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -135,13 +135,20 @@ static struct dst_entry *rxe_find_route6(struct rxe_qp *qp,
 {
 	struct dst_entry *ndst;
 	struct flowi6 fl6 = {};
+	struct sock *sk;
 
 	fl6.flowi6_oif = ndev->ifindex;
 	memcpy(&fl6.saddr, saddr, sizeof(*saddr));
 	memcpy(&fl6.daddr, daddr, sizeof(*daddr));
 	fl6.flowi6_proto = IPPROTO_UDP;
 
-	ndst = ip6_dst_lookup_flow(net, rxe_ns_pernet_sk6(net), &fl6, NULL);
+	/* keep the socket pinned while it is passed to the IPv6 routing
+	 * code which dereferences it outside of rcu_read_lock()
+	 */
+	sk = rxe_ns_pernet_sk6(net);
+	ndst = ip6_dst_lookup_flow(net, sk, &fl6, NULL);
+	if (sk)
+		sock_put(sk);
 	if (IS_ERR(ndst)) {
 		rxe_dbg_qp(qp, "no route to %pI6\n", daddr);
 		return NULL;
@@ -638,9 +645,15 @@ static void rxe_sock_put(struct sock *sk,
 	if (refcount_read(&sk->sk_refcnt) > SK_REF_FOR_TUNNEL) {
 		__sock_put(sk);
 	} else {
+		/*
+		 * Clear the per-net pointer before the socket is released:
+		 * rxe_ns_pernet_sk4/6() take a reference on the socket, but
+		 * new lookups must not find it once it is being released.
+		 */
+		set_sk(net, NULL);
 		rxe_release_udp_tunnel(sk);
-		sk = NULL;
-		set_sk(net, sk);
+		/* drop the reference from rxe_ns_pernet_sk4/6() */
+		sock_put(sk);
 	}
 }
 
@@ -758,11 +771,10 @@ static int rxe_net_ipv4_init(struct net *net)
 	struct sock *sk;
 	struct socket *sock;
 
+	/* returns with a reference held for this device */
 	sk = rxe_ns_pernet_sk4(net);
-	if (sk) {
-		sock_hold(sk);
+	if (sk)
 		return 0;
-	}
 
 	sock = rxe_setup_udp_tunnel(net, htons(ROCE_V2_UDP_DPORT), false);
 	if (IS_ERR(sock)) {
@@ -780,11 +792,10 @@ static int rxe_net_ipv6_init(struct net *net)
 	struct sock *sk;
 	struct socket *sock;
 
+	/* returns with a reference held for this device */
 	sk = rxe_ns_pernet_sk6(net);
-	if (sk) {
-		sock_hold(sk);
+	if (sk)
 		return 0;
-	}
 
 	sock = rxe_setup_udp_tunnel(net, htons(ROCE_V2_UDP_DPORT), true);
 	if (PTR_ERR(sock) == -EAFNOSUPPORT) {
diff --git a/drivers/infiniband/sw/rxe/rxe_ns.c b/drivers/infiniband/sw/rxe/rxe_ns.c
index 64621c89f8bf..0a7d409c0f98 100644
--- a/drivers/infiniband/sw/rxe/rxe_ns.c
+++ b/drivers/infiniband/sw/rxe/rxe_ns.c
@@ -71,6 +71,12 @@ static struct pernet_operations rxe_net_ops = {
 	.size = sizeof(struct rxe_ns_sock),
 };
 
+/*
+ * Return the per-net tunnel socket with a reference taken, or NULL if
+ * the namespace has no tunnel socket.  The reference is taken under
+ * rcu_read_lock() so the socket cannot be freed while the pointer is
+ * still published; the caller must sock_put() it.
+ */
 struct sock *rxe_ns_pernet_sk4(struct net *net)
 {
 	struct rxe_ns_sock *ns_sk = net_generic(net, rxe_pernet_id);
@@ -78,6 +84,8 @@ struct sock *rxe_ns_pernet_sk4(struct net *net)
 
 	rcu_read_lock();
 	sk = rcu_dereference(ns_sk->rxe_sk4);
+	if (sk && !refcount_inc_not_zero(&sk->sk_refcnt))
+		sk = NULL;
 	rcu_read_unlock();
 
 	return sk;
@@ -99,6 +107,8 @@ struct sock *rxe_ns_pernet_sk6(struct net *net)
 
 	rcu_read_lock();
 	sk = rcu_dereference(ns_sk->rxe_sk6);
+	if (sk && !refcount_inc_not_zero(&sk->sk_refcnt))
+		sk = NULL;
 	rcu_read_unlock();
 
 	return sk;

Re: [PATCH v3] RDMA/rxe: Fix use-after-free of the per-net tunnel socket

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/rxe: broken sk_refcnt check and reference leak in rxe_sock_put()
--

commit 4fbcdc28fc0ebeebf87dd5a747900ac13874c9a3
Author: Binbin Deng <18983559317@163.com>

RDMA/rxe: Fix use-after-free of the per-net tunnel socket

This patch fixes a use-after-free of the per-net UDP tunnel socket by
clearing the per-net socket pointer before releasing the tunnel in
rxe_sock_put(), and by taking a reference on sk->sk_refcnt under
rcu_read_lock() in rxe_ns_pernet_sk4() and rxe_ns_pernet_sk6().

> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 53daaf4c1eb2e..ec98133a60055 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -135,13 +135,20 @@ static struct dst_entry *rxe_find_route6(struct rxe_qp *qp,
>  {
>  	struct dst_entry *ndst;
>  	struct flowi6 fl6 = {};
> +	struct sock *sk;
>  
>  	fl6.flowi6_oif = ndev->ifindex;
>  	memcpy(&fl6.saddr, saddr, sizeof(*saddr));
>  	memcpy(&fl6.daddr, daddr, sizeof(*daddr));
>  	fl6.flowi6_proto = IPPROTO_UDP;
>  
> -	ndst = ip6_dst_lookup_flow(net, rxe_ns_pernet_sk6(net), &fl6, NULL);
> +	/* keep the socket pinned while it is passed to the IPv6 routing
> +	 * code which dereferences it outside of rcu_read_lock()
> +	 */
> +	sk = rxe_ns_pernet_sk6(net);
> +	ndst = ip6_dst_lookup_flow(net, sk, &fl6, NULL);
> +	if (sk)
> +		sock_put(sk);
>  	if (IS_ERR(ndst)) {
>  		rxe_dbg_qp(qp, "no route to %pI6\n", daddr);
>  		return NULL;
> @@ -638,9 +645,15 @@ static void rxe_sock_put(struct sock *sk,
>  	if (refcount_read(&sk->sk_refcnt) > SK_REF_FOR_TUNNEL) {
>  		__sock_put(sk);
>  	} else {
> +		/*
> +		 * Clear the per-net pointer before the socket is released:
> +		 * rxe_ns_pernet_sk4/6() take a reference on the socket, but
> +		 * new lookups must not find it once it is being released.
> +		 */
> +		set_sk(net, NULL);
>  		rxe_release_udp_tunnel(sk);
> -		sk = NULL;
> -		set_sk(net, sk);
> +		/* drop the reference from rxe_ns_pernet_sk4/6() */
> +		sock_put(sk);
>  	}
>  }

[Severity: High]
Does this change make the else branch in rxe_sock_put() unreachable and
leak sk->sk_refcnt as well as the UDP tunnel sockets?

When the first RXE device in a network namespace creates a UDP tunnel
socket via rxe_setup_udp_tunnel() in rxe_net_ipv4_init() or
rxe_net_ipv6_init(), sk->sk_refcnt starts at 2 (SK_REF_FOR_TUNNEL). Each
additional device in the namespace increments sk->sk_refcnt by 1 via
rxe_ns_pernet_sk4() or rxe_ns_pernet_sk6().

Both callers of rxe_sock_put() fetch sk via rxe_ns_pernet_sk4() or
rxe_ns_pernet_sk6() right before calling rxe_sock_put():

drivers/infiniband/sw/rxe/rxe_net.c:rxe_net_del() {
	sk = rxe_ns_pernet_sk4(net);
	if (sk)
		rxe_sock_put(sk, rxe_ns_pernet_set_sk4, net);

	sk = rxe_ns_pernet_sk6(net);
	if (sk)
		rxe_sock_put(sk, rxe_ns_pernet_set_sk6, net);
}

drivers/infiniband/sw/rxe/rxe_net.c:rxe_net_init() {
    ...
err_out:
	/* If ipv6 error, release ipv4 resource */
	sk = rxe_ns_pernet_sk4(net);
	if (sk)
		rxe_sock_put(sk, rxe_ns_pernet_set_sk4, net);
    ...
}

Because rxe_ns_pernet_sk4() and rxe_ns_pernet_sk6() now increment
sk->sk_refcnt with refcount_inc_not_zero(), refcount_read(&sk->sk_refcnt)
upon entry to rxe_sock_put() is at least SK_REF_FOR_TUNNEL + 1 (3 when one
device exists, or N + 2 for N > 1 devices).

As a result, in rxe_sock_put():

1. refcount_read(&sk->sk_refcnt) > SK_REF_FOR_TUNNEL is always true, so the
   else branch (set_sk(net, NULL) and rxe_release_udp_tunnel(sk)) is never
   reached when the last RXE device is deleted or when rxe_net_ipv6_init()
   fails.

2. The if branch calls __sock_put(sk) only once, which only drops the
   temporary reference just acquired by the caller's rxe_ns_pernet_sk4() or
   rxe_ns_pernet_sk6() call without dropping the per-device reference taken
   in rxe_net_ipv4_init() or rxe_net_ipv6_init(). Each device addition and
   removal cycle therefore leaks a reference on struct sock.

3. In addition, because rxe_find_route6() also increments sk->sk_refcnt via
   rxe_ns_pernet_sk6(net) and drops it with sock_put(sk), any concurrent
   rxe_find_route6() in flight during rxe_net_del() of the last device will
   elevate sk->sk_refcnt. Even if the threshold in rxe_sock_put() were
   adjusted for the caller's getter reference, wouldn't a concurrent route
   lookup cause rxe_sock_put() to take the if branch and skip releasing the
   UDP tunnel, while rxe_find_route6()'s subsequent sock_put(sk) only
   decrements sk->sk_refcnt without releasing the tunnel?

[ ... ]

> diff --git a/drivers/infiniband/sw/rxe/rxe_ns.c b/drivers/infiniband/sw/rxe/rxe_ns.c
> index 64621c89f8bf1..0a7d409c0f981 100644
> --- a/drivers/infiniband/sw/rxe/rxe_ns.c
> +++ b/drivers/infiniband/sw/rxe/rxe_ns.c
> @@ -71,6 +71,12 @@ static struct pernet_operations rxe_net_ops = {
>  	.size = sizeof(struct rxe_ns_sock),
>  };
>  
> +/*
> + * Return the per-net tunnel socket with a reference taken, or NULL if
> + * the namespace has no tunnel socket.  The reference is taken under
> + * rcu_read_lock() so the socket cannot be freed while the pointer is
> + * still published; the caller must sock_put() it.
> + */
>  struct sock *rxe_ns_pernet_sk4(struct net *net)
>  {
>  	struct rxe_ns_sock *ns_sk = net_generic(net, rxe_pernet_id);
> @@ -78,6 +84,8 @@ struct sock *rxe_ns_pernet_sk4(struct net *net)
>  
>  	rcu_read_lock();
>  	sk = rcu_dereference(ns_sk->rxe_sk4);
> +	if (sk && !refcount_inc_not_zero(&sk->sk_refcnt))
> +		sk = NULL;
>  	rcu_read_unlock();
>  
>  	return sk;
> @@ -99,6 +107,8 @@ struct sock *rxe_ns_pernet_sk6(struct net *net)
>  
>  	rcu_read_lock();
>  	sk = rcu_dereference(ns_sk->rxe_sk6);
> +	if (sk && !refcount_inc_not_zero(&sk->sk_refcnt))
> +		sk = NULL;
>  	rcu_read_unlock();
>  
>  	return sk;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019043205.466134-1-18983559317@163.com?part=1