[PATCH net] rds_tcp: close NULL deref window in rds_tcp_set_callbacks

[PATCH net] rds_tcp: close NULL deref window in rds_tcp_set_callbacks

[Maoyi Xie]

rds_tcp_set_callbacks() links a new rds_tcp_connection onto
rds_tcp_tc_list under rds_tcp_tc_list_lock. It releases the
lock, then assigns tc->t_sock = sock outside the lock.

rds_tcp_tc_info() and rds6_tcp_tc_info() walk rds_tcp_tc_list
under the same lock. Both dereference tc->t_sock->sk without
a NULL check.

A reader can acquire rds_tcp_tc_list_lock between the writer's
spin_unlock and the t_sock store. It then sees a list entry
whose t_sock is NULL. The dereference of tc->t_sock->sk is a
NULL access.

Move tc->t_sock = sock inside rds_tcp_tc_list_lock, before
list_add_tail. A reader holding the lock then observes the
linkage and the t_sock store together.

The restore path is safe. rds_tcp_restore_callbacks() does
list_del_init inside the lock. The matching tc->t_sock = NULL
after unlink is harmless to readers holding the lock.

Fixes: 70041088e3b9 ("RDS: Add TCP transport to RDS")
Suggested-by: Simon Horman <horms@kernel.org>
Signed-off-by: Maoyi Xie <maoyi.xie@ntu.edu.sg>
---
 net/rds/tcp.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 654e23d13..5830b31a1 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -198,8 +198,13 @@ void rds_tcp_set_callbacks(struct socket *sock, struct rds_conn_path *cp)
 	rdsdebug("setting sock %p callbacks to tc %p\n", sock, tc);
 	write_lock_bh(&sock->sk->sk_callback_lock);
 
-	/* done under the callback_lock to serialize with write_space */
+	/* done under the callback_lock to serialize with write_space.
+	 * Set t_sock inside rds_tcp_tc_list_lock so readers walking
+	 * rds_tcp_tc_list under the same lock cannot observe an
+	 * entry whose t_sock is NULL.
+	 */
 	spin_lock(&rds_tcp_tc_list_lock);
+	tc->t_sock = sock;
 	list_add_tail(&tc->t_list_item, &rds_tcp_tc_list);
 #if IS_ENABLED(CONFIG_IPV6)
 	rds6_tcp_tc_count++;
@@ -211,8 +216,6 @@ void rds_tcp_set_callbacks(struct socket *sock, struct rds_conn_path *cp)
 	/* accepted sockets need our listen data ready undone */
 	if (sock->sk->sk_data_ready == rds_tcp_listen_data_ready)
 		sock->sk->sk_data_ready = sock->sk->sk_user_data;
-
-	tc->t_sock = sock;
 	if (!tc->t_rtn)
 		tc->t_rtn = net_generic(sock_net(sock->sk), rds_tcp_netid);
 	tc->t_cpath = cp;

base-commit: b266bacba796ff5c4dcd2ae2fc08aacf7ab39153
-- 
2.34.1

Re: [PATCH net] rds_tcp: close NULL deref window in rds_tcp_set_callbacks

[Allison Henderson]

On Tue, 2026-05-12 at 22:28 +0800, Maoyi Xie wrote:
> rds_tcp_set_callbacks() links a new rds_tcp_connection onto
> rds_tcp_tc_list under rds_tcp_tc_list_lock. It releases the
> lock, then assigns tc->t_sock = sock outside the lock.
> 
> rds_tcp_tc_info() and rds6_tcp_tc_info() walk rds_tcp_tc_list
> under the same lock. Both dereference tc->t_sock->sk without
> a NULL check.
> 
> A reader can acquire rds_tcp_tc_list_lock between the writer's
> spin_unlock and the t_sock store. It then sees a list entry
> whose t_sock is NULL. The dereference of tc->t_sock->sk is a
> NULL access.
> 
> Move tc->t_sock = sock inside rds_tcp_tc_list_lock, before
> list_add_tail. A reader holding the lock then observes the
> linkage and the t_sock store together.
> 
> The restore path is safe. rds_tcp_restore_callbacks() does
> list_del_init inside the lock. The matching tc->t_sock = NULL
> after unlink is harmless to readers holding the lock.
> 
> Fixes: 70041088e3b9 ("RDS: Add TCP transport to RDS")
> Suggested-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Maoyi Xie <maoyi.xie@ntu.edu.sg>

Hi Maoyi,
This fix looks fine to me.  Thanks for the catch.

Reviewed-by: Allison Henderson <achender@kernel.org>

> ---
>  net/rds/tcp.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index 654e23d13..5830b31a1 100644
> --- a/net/rds/tcp.c
> +++ b/net/rds/tcp.c
> @@ -198,8 +198,13 @@ void rds_tcp_set_callbacks(struct socket *sock, struct rds_conn_path *cp)
>  	rdsdebug("setting sock %p callbacks to tc %p\n", sock, tc);
>  	write_lock_bh(&sock->sk->sk_callback_lock);
>  
> -	/* done under the callback_lock to serialize with write_space */
> +	/* done under the callback_lock to serialize with write_space.
> +	 * Set t_sock inside rds_tcp_tc_list_lock so readers walking
> +	 * rds_tcp_tc_list under the same lock cannot observe an
> +	 * entry whose t_sock is NULL.
> +	 */
>  	spin_lock(&rds_tcp_tc_list_lock);
> +	tc->t_sock = sock;
>  	list_add_tail(&tc->t_list_item, &rds_tcp_tc_list);
>  #if IS_ENABLED(CONFIG_IPV6)
>  	rds6_tcp_tc_count++;
> @@ -211,8 +216,6 @@ void rds_tcp_set_callbacks(struct socket *sock, struct rds_conn_path *cp)
>  	/* accepted sockets need our listen data ready undone */
>  	if (sock->sk->sk_data_ready == rds_tcp_listen_data_ready)
>  		sock->sk->sk_data_ready = sock->sk->sk_user_data;
> -
> -	tc->t_sock = sock;
>  	if (!tc->t_rtn)
>  		tc->t_rtn = net_generic(sock_net(sock->sk), rds_tcp_netid);
>  	tc->t_cpath = cp;
> 
> base-commit: b266bacba796ff5c4dcd2ae2fc08aacf7ab39153