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

[sashiko-bot@kernel.org]

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