[PATCH net] net/rds: fix out-of-bounds write in rds_conn_peer_gen_update()

[Paula Moutafian <paula@bynar.io>]

rds_conn_peer_gen_update(), rds_start_mprds() and rds_check_all_paths()
all iterate conn->c_path[] up to RDS_MPATH_WORKERS (8) or conn->c_npaths,
but c_path is allocated with only

	npaths = (trans->t_mp_capable ? RDS_MPATH_WORKERS : 1)

entries in __rds_conn_create() (net/rds/connection.c).  Only the TCP
transport sets t_mp_capable, so for the IB/RDMA transport (and the loop
transport) exactly one rds_conn_path is allocated.

c_npaths is derived from the peer via the RDS_EXTHDR_NPATHS handshake
extension header and is only clamped to RDS_MPATH_WORKERS, never to the
transport's actual allocation.  A remote peer on an RDS/IB (RoCE) fabric
can therefore complete the unauthenticated handshake and, by sending a
second RDS_EXTHDR_GEN_NUM with a changed generation number, drive
rds_conn_peer_gen_update() to walk c_path[1..7] past the end of a
one-element allocation -- taking cp_lock, writing cp_next_tx_seq /
cp_next_rx_seq and walking cp_retrans on neighbouring slab objects.
rds_start_mprds() and rds_check_all_paths() are reachable the same way
via a peer-supplied c_npaths > 1; note rds_check_all_paths() is a
do/while and dereferences c_path[0] before testing the bound.

Reproduced against an unmodified 7.2.0 KASAN build, triggered from a
hand-rolled RDS/IB peer completing the handshake and sending two probes
with differing RDS_EXTHDR_GEN_NUM:

BUG: KASAN: slab-out-of-bounds in do_raw_spin_lock+0x55/0x9a
Write of size 4 at addr ff1100000425a624 by task ksoftirqd/0/13
Call Trace:
 do_raw_spin_lock+0x55/0x9a
 _raw_spin_lock_irqsave+0x12/0x18
 rds_recv_hs_exthdrs+0x34a/0x52a
 rds_recv_incoming+0x5a8/0xb33
 rds_ib_recv_cqe_handler+0xda1/0x12b6
 poll_rcq+0x8e/0xb1
 rds_ib_tasklet_fn_recv+0x1c4/0x348
Allocated by task 10:
 __rds_conn_create+0x5b3/0x168f
 rds_conn_create+0x18/0x1b
 rds_ib_cm_handle_connect+0x486/0xa35
The buggy address is located 44 bytes to the right of
 allocated 504-byte region, cache kmalloc-512

The corrupted neighbour's cp_retrans.next is then dereferenced on the
next line, producing a fatal GPF (RIP: rds_recv_hs_exthdrs+0x3ba/0x52a,
KASAN: null-ptr-deref) -- a crash, not merely a detected access.

Bound every loop by the number of paths actually allocated, mirroring
the npaths computation already used in rds_conn_destroy() and
rds_conn_message_info_cmn() (net/rds/connection.c).  For t_mp_capable
transports the behaviour is unchanged; for single-path transports the
loops now stay within the one allocated rds_conn_path.

rds_conn_peer_gen_update() and rds_start_mprds() (net/rds/recv.c) were
both correct when written, with c_path still a fixed
RDS_MPATH_WORKERS-element array:

commit 905dd4184e07 ("RDS: TCP: Track peer's connection generation number")
commit 5916e2c1554f ("RDS: TCP: Enable multipath RDS for TCP")

They became wrong when the allocation was made conditional on
t_mp_capable without updating either loop:

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

rds_check_all_paths() (net/rds/connection.c) is unrelated to that
regression: it was added new, three years later, and was unbounded
from the moment it was written:

commit 9ef845f894c9 ("rds: If one path needs re-connection, check all and re-connect")

Fixes: 840df162b3eb ("rds: reduce memory footprint for RDS when transport is RDMA")
Fixes: 9ef845f894c9 ("rds: If one path needs re-connection, check all and re-connect")
Cc: stable@vger.kernel.org
Assisted-by: Bynario AI
Signed-off-by: Paula Moutafian <paula@bynar.io>
---
 net/rds/connection.c |  3 ++-
 net/rds/recv.c       | 10 ++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 7c8ab8e973e1..282e320516f5 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -976,11 +976,12 @@ EXPORT_SYMBOL_GPL(rds_conn_path_connect_if_down);
  */
 void rds_check_all_paths(struct rds_connection *conn)
 {
+	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
 	int i = 0;
 
 	do {
 		rds_conn_path_connect_if_down(&conn->c_path[i]);
-	} while (++i < conn->c_npaths);
+	} while (++i < conn->c_npaths && i < npaths);
 }
 
 void rds_conn_connect_if_down(struct rds_connection *conn)
diff --git a/net/rds/recv.c b/net/rds/recv.c
index cf3884d87931..3813aa323731 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -133,15 +133,16 @@ static void rds_recv_rcvbuf_delta(struct rds_sock *rs, struct sock *sk,
 static void rds_conn_peer_gen_update(struct rds_connection *conn,
 				     u32 peer_gen_num)
 {
-	int i;
+	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
 	struct rds_message *rm, *tmp;
 	unsigned long flags;
+	int i;
 
 	WARN_ON(conn->c_trans->t_type != RDS_TRANS_TCP);
 	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 < npaths; i++) {
 				struct rds_conn_path *cp;
 
 				cp = &conn->c_path[i];
@@ -285,12 +286,13 @@ static void rds_recv_hs_exthdrs(struct rds_header *hdr,
  */
 static void rds_start_mprds(struct rds_connection *conn)
 {
-	int i;
+	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
 	struct rds_conn_path *cp;
+	int i;
 
 	if (conn->c_npaths > 1 &&
 	    rds_addr_cmp(&conn->c_laddr, &conn->c_faddr) < 0) {
-		for (i = 0; i < conn->c_npaths; i++) {
+		for (i = 0; i < conn->c_npaths && i < npaths; i++) {
 			cp = &conn->c_path[i];
 			rds_conn_path_connect_if_down(cp);
 		}

base-commit: 564973a259ec76f2dad0853420e7034cc43994c4
-- 
2.50.1 (Apple Git-155)