Re: [PATCH net v4] rds: filter RDS_INFO_* getsockopt by caller's netns

[Simon Horman <horms@kernel.org>]

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
---
rds: filter RDS_INFO_* getsockopt by caller's netns

This commit adds network namespace filtering to the RDS_INFO_* getsockopt
handlers to restrict visibility of sockets and connections. It removes global
counters and replaces them with a per-netns count gathered from a first pass
over the lists.

> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index 76f625986a7f2..ace52d3ce0597 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c

[ ... ]

> @@ -806,6 +811,7 @@ static void rds_sock_info(struct socket *sock, unsigned int len,
>  			  struct rds_info_iterator *iter,
>  			  struct rds_info_lengths *lens)
>  {
> +	struct net *net = sock_net(sock->sk);
>  	struct rds_info_socket sinfo;
>  	unsigned int cnt = 0;
>  	struct rds_sock *rs;
> @@ -814,12 +820,22 @@ static void rds_sock_info(struct socket *sock, unsigned int len,
>  
>  	spin_lock_bh(&rds_sock_lock);
>  
> -	if (len < rds_sock_count) {
> -		cnt = rds_sock_count;
> -		goto out;
> +	/* First pass: count entries visible in the caller's netns. */
> +	list_for_each_entry(rs, &rds_sock_list, rs_item) {
> +		if (!net_eq(sock_net(rds_rs_to_sk(rs)), net))
> +			continue;
> +		if (!ipv6_addr_v4mapped(&rs->rs_bound_addr))
> +			continue;
> +		cnt++;
>  	}
>  
> +	if (len < cnt)
> +		goto out;
> +
>  	list_for_each_entry(rs, &rds_sock_list, rs_item) {
> +		/* Only show sockets in the caller's netns. */
> +		if (!net_eq(sock_net(rds_rs_to_sk(rs)), net))
> +			continue;
>  		/* This option only supports IPv4 sockets. */
>  		if (!ipv6_addr_v4mapped(&rs->rs_bound_addr))
>  			continue;
> @@ -832,7 +848,6 @@ static void rds_sock_info(struct socket *sock, unsigned int len,
>  		sinfo.inum = sock_i_ino(rds_rs_to_sk(rs));
>  
>  		rds_info_copy(iter, &sinfo, sizeof(sinfo));

Could a concurrent bind cause this second pass to copy more items than we
counted in the first pass?

If rds_bind() updates rs_bound_addr concurrently without holding
rds_sock_lock, an unbound socket could be skipped during the first pass but
match in the second pass.

Because the second pass does not limit the number of copied items to the
length checked after the first pass, rds_info_copy() could be called more
times than the user-supplied buffer can handle.

For example, if a caller provides a length of 0, the iter pages array may
be NULL. If the first pass computes cnt = 0, the length check (len < cnt)
evaluates to false and the second pass executes. If a socket is bound
concurrently and matches the second pass, could this lead to an out-of-bounds
read or a crash when rds_info_copy() dereferences iter->pages?