Re: [PATCH] rds: fix out-of-bounds conn_path walk in peer gen update

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 1 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] The patch only bounds the loop in
  rds_conn_peer_gen_update().
- [Medium] The Fixes: tag names ba3d1f480c7a ("net/rds: size a
  connection's path set by the transport it ends up with", Sep 2026),…

--

rds: fix out-of-bounds conn_path walk in peer gen update

This patch limits the loop in rds_conn_peer_gen_update() to the number of
rds_conn_path entries allocated for the connection's transport. With it, a
non-mp-capable (IB) connection only walks its single c_path slot when the
peer announces a new generation number.

> Cc: stable@vger.kernel.org
> Fixes: ba3d1f480c7a ("net/rds: size a connection's path set by the transport it ends up with")

[Severity: Medium]
Is this the right commit for the Fixes: tag?

git blame shows that the RDS_MPATH_WORKERS loop in
rds_conn_peer_gen_update() dates from 905dd4184e07 (2016).

The single-entry c_path allocation for non-mp-capable transports came from
840df162b3eb ("rds: reduce memory footprint for RDS when transport is
RDMA", 2017):

net/rds/connection.c:__rds_conn_create() {
    ...
	int npaths = (trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
    ...
}

ba3d1f480c7a only adds a second npaths computation after the loopback
transport swap. That changes how many paths get initialised, not the size
of c_path. Its own commit message says "The c_path array stays sized for
the caller's transport". IB conns had one slot before that commit and
still have one after it.

Stable tooling uses the Fixes: tag to pick target trees, so this tag could
leave out LTS trees from 2017 onward that have the out-of-bounds write.
Should it be this instead?

Fixes: 840df162b3eb ("rds: reduce memory footprint for RDS when transport is RDMA")

> diff --git a/net/rds/recv.c b/net/rds/recv.c
> index 6204e577a90ae..afc810a2e3cf9 100644
> --- a/net/rds/recv.c
> +++ b/net/rds/recv.c
> @@ -144,7 +144,8 @@ static void rds_conn_peer_gen_update(struct rds_connection *conn,
>  	if (peer_gen_num != 0) {
>  		if (conn->c_peer_gen_num != 0 &&
>  		    peer_gen_num != conn->c_peer_gen_num) {
> -			for (i = 0; i < RDS_MPATH_WORKERS; i++) {
> +			for (i = 0; i < (conn->c_trans->t_mp_capable ?
> +			     RDS_MPATH_WORKERS : 1); i++) {

[Severity: Critical]
This limits the walk here. Can the same handshake packet still overrun the
single-entry c_path[] of an IB connection through c_npaths?

rds_recv_incoming() calls rds_recv_hs_exthdrs() for handshake pings and
pongs on every transport, and nothing on that path checks t_mp_capable.
The ping sysctl does not gate the pong branch:

net/rds/recv.c:rds_recv_incoming() {
    ...
	if (be16_to_cpu(inc->i_hdr.h_dport) ==  RDS_FLAG_PROBE_PORT &&
	    inc->i_hdr.h_sport == 0) {
		rds_recv_hs_exthdrs(&inc->i_hdr, cp->cp_conn);
		/* if this is a handshake pong, start multipath if necessary */
		rds_start_mprds(cp->cp_conn);
    ...
}

rds_recv_hs_exthdrs() takes the RDS_EXTHDR_NPATHS value from the peer and
caps it at RDS_MPATH_WORKERS, not at the allocated path count. It then
stores the result:

net/rds/recv.c:rds_recv_hs_exthdrs() {
    ...
		case RDS_EXTHDR_NPATHS:
			new_npaths = min_t(int, RDS_MPATH_WORKERS,
					   be16_to_cpu(buffer.rds_npaths));
    ...
	conn->c_npaths = max_t(int, new_npaths, 1);
    ...
}

This lets c_npaths reach 8 on an IB conn where __rds_conn_create()
allocated only one c_path entry.

In the same receive call, when laddr < faddr, rds_start_mprds() walks
c_npaths entries:

net/rds/recv.c:rds_start_mprds() {
    ...
		for (i = 0; i < conn->c_npaths; i++) {
			cp = &conn->c_path[i];
			rds_conn_path_connect_if_down(cp);
		}
    ...
}

When laddr > faddr, c_npaths still stays inflated. A later rds_sendmsg()
reaches rds_check_all_paths() whenever the selected path is down:

net/rds/connection.c:rds_check_all_paths() {
    ...
	do {
		rds_conn_path_connect_if_down(&conn->c_path[i]);
	} while (++i < conn->c_npaths);
}

For c_path[1..7], rds_conn_path_connect_if_down() reads cp->cp_conn from
the neighbouring slab object and dereferences it in rds_destroy_pending().
It also does test_and_set_bit() on cp->cp_flags and calls
queue_delayed_work() with cp->cp_wq and cp->cp_conn_w from that same
memory.

rds_send_probe() only sends these extension headers when t_mp_capable is
set, but the receive side applies them on every transport.

Would it be better to skip rds_recv_hs_exthdrs() and rds_start_mprds() for
non-mp-capable transports, or to clamp c_npaths to the allocated path
count? Either would cover all of these walks.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008164041.961961-1-bsdhenrymartin%40gmail.com