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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/rds: fix out-of-bounds write in rds_conn_peer_gen_update()

This clamps the peer-supplied RDS_EXTHDR_NPATHS value in
rds_recv_hs_exthdrs() to the number of rds_conn_path entries the transport
actually allocated, and gives the loop in rds_conn_peer_gen_update() the
same bound instead of the fixed RDS_MPATH_WORKERS constant.

Reference to the earlier posting:
[v1] https://lore.kernel.org/netdev/213829b7380f1fe12aed2f2ae9ba33c2870addd5.camel@xxxxxxxxxx/T/#t

> diff --git a/net/rds/recv.c b/net/rds/recv.c
> index cf3884d879319..c7f575bad91c8 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;

[Severity: High]
This isn't a bug introduced by this patch, but is
conn->c_trans->t_mp_capable really equivalent to the count used at the
allocation site?

__rds_conn_create() derives npaths from the incoming trans, allocates
c_path plus one ordered workqueue per path, and only afterwards may replace
the transport:

net/rds/connection.c:__rds_conn_create() {
	int npaths = (trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
	...
				trans = &rds_loop_transport;
	...
	conn->c_trans = trans;
	for (i = 0; i < npaths; i++) {
		...
		cp->cp_wq = alloc_ordered_workqueue(...);

For an outgoing TCP connection to a local non-loopback address
(rds_tcp_transport has t_prefer_loopback set) the conn therefore ends up
with 8 paths and 8 workqueues while conn->c_trans is rds_loop_transport,
which has t_mp_capable clear.

rds_conn_destroy() recomputes the count exactly the way this hunk does:

net/rds/connection.c:rds_conn_destroy() {
	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
	...
	for (i = 0; i < npaths; i++) {
		cp = &conn->c_path[i];
		rds_conn_path_destroy(cp);
	...
	kfree(conn->c_path);

Does that leak the cp_wq workqueues belonging to paths 1..7 in this case?
Note rds_conn_path_destroy() also returns early before destroy_workqueue():

net/rds/connection.c:rds_conn_path_destroy() {
	if (!cp->cp_transport_data)
		return;
	...
	if (cp->cp_wq != rds_wq) {
		destroy_workqueue(cp->cp_wq);

so simply widening the destroy loop would not reach the workqueues either.
The failure path in __rds_conn_create() uses the pre-swap npaths to destroy
all the workqueues, which suggests npaths, not the post-swap transport, is
the real allocation count.

The derivation is harmless in this patch (1 is always <= the allocation),
but would recording the allocated path count in struct rds_connection and
using that here avoid duplicating the derivation in a third place?

>  
>  	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];

[Severity: High]
The new bound stops the walk past the end of c_path[], but does the rest of
this function stay reachable from the same non-TCP peer described in the
changelog?

rds_recv_incoming() dispatches to the handshake parser purely on
peer-supplied header fields, with no transport check:

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);

and rds_recv_hs_exthdrs() calls rds_conn_peer_gen_update() unconditionally.
rds_ib_process_recv() hands the wire header straight to rds_recv_incoming(),
and rds_ib_transport sets .t_type = RDS_TRANS_IB, so the WARN_ON above is
true for every crafted probe on an IB connection.

Can that flood dmesg from a remote peer, or panic the box on
panic_on_warn=1 kernels? Would ratelimiting it, or refusing handshake
extension headers on non mp-capable transports, be preferable?

With npaths == 1 the i == 0 iteration is still executed. Two probes with
differing non-zero RDS_EXTHDR_GEN_NUM let the same peer reset
c_path[0].cp_next_tx_seq to 1 and cp_next_rx_seq to 0 and set RDS_MSG_FLUSH
on every message queued on c_path[0].cp_retrans of a live IB connection.

rds_send_probe() only ever emits these headers for t_mp_capable transports:

net/rds/send.c:rds_send_probe() {
	if (RDS_HS_PROBE(be16_to_cpu(sport), be16_to_cpu(dport)) &&
	    cp->cp_conn->c_trans->t_mp_capable) {

so on IB both NPATHS and GEN_NUM are always forged. Would an early return in
rds_recv_hs_exthdrs() for transports without t_mp_capable remove both the
WARN and the c_path[0] state mutation, and make the new bound here
unnecessary?

[Severity: Medium]
This is a pre-existing issue, but the compare-then-store of
conn->c_peer_gen_num in this function happens outside any lock — cp_lock
only covers the per-path queue walk:

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

For a t_mp_capable conn each path has its own cp_wq and receive worker, so
two lanes can run rds_recv_hs_exthdrs(cp->cp_conn) for the same conn at the
same time. Can two probes carrying different generation numbers then either
double-flush or skip the RDS_MSG_FLUSH marking entirely, leaving stale
cp_retrans messages after a peer restart?

> @@ -210,6 +211,7 @@ static void rds_recv_hs_exthdrs(struct rds_header *hdr,
>  	u32 new_peer_gen_num = 0;
>  	int new_npaths;
>  	bool fan_out;
> +	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);

[Severity: Low]
This isn't a bug, but the new declaration is the longest one in the block
and is placed last, which does not follow the reverse xmas tree ordering
netdev asks for. The first hunk of this patch moves int i; below
unsigned long flags; for exactly that reason, so the two hunks disagree.

>  
>  	new_npaths = conn->c_npaths;
>  
> @@ -221,7 +223,7 @@ static void rds_recv_hs_exthdrs(struct rds_header *hdr,
>  		/* Process extension header here */
>  		switch (type) {
>  		case RDS_EXTHDR_NPATHS:
> -			new_npaths = min_t(int, RDS_MPATH_WORKERS,
> +			new_npaths = min_t(int, npaths,
>  					   be16_to_cpu(buffer.rds_npaths));
>  			break;
>  		case RDS_EXTHDR_GEN_NUM:

[Severity: Medium]
This is a pre-existing issue, but conn->c_npaths — now the single
authoritative bound for rds_start_mprds() and rds_check_all_paths() — is
read, tested and published here with no lock, no atomics and no
READ_ONCE()/WRITE_ONCE():

net/rds/recv.c:rds_recv_hs_exthdrs() {
	new_npaths = conn->c_npaths;
	...
	if (new_npaths > 1 && new_npaths != conn->c_npaths) {
	...
	conn->c_npaths = max_t(int, new_npaths, 1);

Since each path of an mp-capable conn has its own receive worker, two lanes
can be in this function for the same conn concurrently. Can one CPU publish
c_npaths = 8 and take the fan-out branch while a second CPU, processing a
probe with no NPATHS extension, writes c_npaths = 1 and silently reverts it
while lanes are still coming up?

The lane mapping in rds_sendmsg() reads the same field with no
synchronisation:

net/rds/send.c:rds_sendmsg() {
		cpath = &conn->c_path[RDS_MPATH_HASH(rs, conn->c_npaths ? : 1)];

so the local and peer lane hashing could disagree. Every stored value still
passes through max_t(int, new_npaths, 1) with new_npaths already clamped, so
this looks like a consistency problem rather than an indexing one — is that
the intended reading?

The comment above rds_recv_hs_exthdrs() only promises serialisation against
other receives on "this specific conn", which for mprds is per-path. Would
WRITE_ONCE()/READ_ONCE() plus per-conn serialisation of handshake
processing be the right follow-up?