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

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

[Henry Martin]

A connection on the IB transport (t_mp_capable unset) is allocated
with exactly one rds_conn_path slot, yet the peer's genetic number
update routine is hardcoded to walk RDS_MPATH_WORKERS entries whenever
the peer announces a non-zero gen number that differs from the one we
cached.  Everything past the first entry lands on whatever happens to
follow the single-slot array in the same slab slot group, and the loop
then reads and modifies struct fields there (lock acquired,
cp_next_tx_seq cleared, retransmit list inspected).  A malicious peer
can therefore force consecutive out-of-bounds writes into neighbouring
kmalloc-512 objects (KASAN: "Write of size 4 in
_raw_spin_lock_irqsave").

The allocation and the walk now agree on the same expression
(trans->t_mp_capable ? RDS_MPATH_WORKERS : 1), so the loop can never
outlive the array regardless of what the peer reports.

This vulnerability was discovered by Tencent CodeBuddy Security.

Cc: stable@vger.kernel.org
Fixes: ba3d1f480c7a ("net/rds: size a connection's path set by the transport it ends up with")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 net/rds/recv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -144,7 +144,8 @@
 	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++) {
 				struct rds_conn_path *cp;

 				cp = &conn->c_path[i];
--
2.43.7

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/rds: race condition in rds_conn_peer_gen_update causes sequence number reset
--

commit 717052475aa8589026dc0030beaf8862f5697d69
Author: Henry Martin <bsdhenrymartin@gmail.com>

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

This commit modifies rds_conn_peer_gen_update() to bound the loop over
c_path entries. It checks whether the transport is multipath capable and
walks either RDS_MPATH_WORKERS entries or just 1, preventing out-of-bounds
memory accesses on IB transports.

> 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++) {
>  				struct rds_conn_path *cp;
>  
>  				cp = &conn->c_path[i];

[Severity: High]
Does this code have a race condition that could cause sequence numbers to be
reset during active transmission?

When a peer reboots and establishes multiple multipath TCP connections
concurrently, handshake pings arrive on these paths simultaneously.

Because rds_conn_peer_gen_update() lacks connection-level serialization
across paths, multiple threads can evaluate the generation number
simultaneously:

	if (conn->c_peer_gen_num != 0 &&
	    peer_gen_num != conn->c_peer_gen_num) {

If Thread A and Thread B both evaluate this condition as true and enter the
block, Thread A might complete the reset loop and the path could resume
active transmission.

If Thread B is delayed or preempted, could it then execute the loop and
reset cp_next_tx_seq and cp_next_rx_seq after Thread A has finished?

This would break sequence monotonicity and fragment reassembly, leading
to packet drops and connection stalling.

Furthermore, is the update to the connection state protected against
this race?

net/rds/recv.c:rds_conn_peer_gen_update() {
    ...
		}
		conn->c_peer_gen_num = peer_gen_num;
	}
}

This unprotected write to the shared connection state doesn't seem to
prevent concurrent threads from observing the old value and incorrectly
assuming they must execute the reset block.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008164041.961961-1-bsdhenrymartin@gmail.com?part=1

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

[Allison Henderson]

On Fri, 2026-10-09 at 00:40 +0800, Henry Martin wrote:
> A connection on the IB transport (t_mp_capable unset) is allocated
> with exactly one rds_conn_path slot, yet the peer's genetic number
> update routine is hardcoded to walk RDS_MPATH_WORKERS entries whenever
> the peer announces a non-zero gen number that differs from the one we
> cached.  Everything past the first entry lands on whatever happens to
> follow the single-slot array in the same slab slot group, and the loop
> then reads and modifies struct fields there (lock acquired,
> cp_next_tx_seq cleared, retransmit list inspected).  A malicious peer
> can therefore force consecutive out-of-bounds writes into neighbouring
> kmalloc-512 objects (KASAN: "Write of size 4 in
> _raw_spin_lock_irqsave").
> 
> The allocation and the walk now agree on the same expression
> (trans->t_mp_capable ? RDS_MPATH_WORKERS : 1), so the loop can never
> outlive the array regardless of what the peer reports.
> 
> This vulnerability was discovered by Tencent CodeBuddy Security.

Hi Henry,

Please see my reply to the NPATHS patch:
https://lore.kernel.org/netdev/20261008163843.957325-1-bsdhenrymartin@gmail.com/

We should fold this patch into a single v2 from that thread.

Thank you!
Allison

pw-bot: cr


> 
> Cc: stable@vger.kernel.org
> Fixes: ba3d1f480c7a ("net/rds: size a connection's path set by the transport it ends up with")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> ---
>  net/rds/recv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- a/net/rds/recv.c
> +++ b/net/rds/recv.c
> @@ -144,7 +144,8 @@
>  	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++) {
>  				struct rds_conn_path *cp;
> 
>  				cp = &conn->c_path[i];
> --
> 2.43.7

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

[netdev-bot+sashiko]

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