Re: [PATCH] rds: fix out-of-bounds conn_path access via handshake NPATHS

[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] This fix leaves the same out-of-bounds walk reachable
  through another lane loop.
- [Medium] The Fixes: tag names the wrong commit.

--

rds: fix out-of-bounds conn_path access via handshake NPATHS

This patch clamps the RDS_EXTHDR_NPATHS value the peer sends in
rds_recv_hs_exthdrs() to the number of rds_conn_path slots the transport
was allocated with. As a result, IB connections (t_mp_capable unset)
negotiate at most one lane. The aim is to stop fan-out paths such as
rds_start_mprds() from walking past the single allocated c_path entry.

> 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?

All ba3d1f480c7a does is recompute npaths a second time after the
loopback swap in __rds_conn_create():

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

Its own message says "The c_path array stays sized for the caller's
transport". So it doesn't change how many slots an IB connection gets.

IB connections have had a one-element c_path since 840df162b3eb ("rds:
reduce memory footprint for RDS when transport is RDMA"). That commit
replaced the inline c_path[RDS_MPATH_WORKERS] array with an allocation
sized by:

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

The NPATHS clamp to RDS_MPATH_WORKERS and the rds_start_mprds() loop over
c_npaths are even older (5916e2c1554f and 69b92b5b7419).

Stable tooling chooses target trees from the Fixes: tag. Would LTS trees
that carry 840df162b3eb but not ba3d1f480c7a miss this fix? Should the
tag be:

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..c9528812edef1 100644
> --- a/net/rds/recv.c
> +++ b/net/rds/recv.c
> @@ -224,7 +224,8 @@ 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, conn->c_trans->t_mp_capable ?
> +					   RDS_MPATH_WORKERS : 1,
>  					   be16_to_cpu(buffer.rds_npaths));
>  			break;
>  		case RDS_EXTHDR_GEN_NUM:

[Severity: Critical]
This clamps NPATHS. Can the RDS_EXTHDR_GEN_NUM case just below it still
cause the same out-of-bounds walk on IB connections?

rds_recv_hs_exthdrs() always calls rds_conn_peer_gen_update() at the end.
The call does not depend on c_npaths, fan_out or t_mp_capable:

    conn->c_ping_triggered = 0;
    rds_conn_peer_gen_update(conn, new_peer_gen_num);

rds_conn_peer_gen_update() then loops over a fixed RDS_MPATH_WORKERS
lanes:

net/rds/recv.c:rds_conn_peer_gen_update() {
    ...
	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++) {
				struct rds_conn_path *cp;

				cp = &conn->c_path[i];
				spin_lock_irqsave(&cp->cp_lock, flags);
				cp->cp_next_tx_seq = 1;
				cp->cp_next_rx_seq = 0;
				list_for_each_entry_safe(rm, tmp,
							 &cp->cp_retrans,
							 m_conn_item) {
					set_bit(RDS_MSG_FLUSH, &rm->m_flags);
				}
    ...
}

The WARN_ON() only prints a warning. It doesn't return.

On IB, __rds_conn_create() allocates a single c_path slot. A peer can
send two handshake probes carrying different nonzero GEN_NUM values. A
pong (h_sport == 0, h_dport == RDS_FLAG_PROBE_PORT) has no sysctl or
transport gate. A ping also works if rds_sysctl_ping_enable is set. The
call chain is:

rds_ib_process_recv()
  rds_recv_incoming()
    rds_recv_hs_exthdrs()
      rds_conn_peer_gen_update()

The first probe stores c_peer_gen_num, and the second enters the loop.
For i=1..7, would the loop take a spinlock and write cp_next_tx_seq and
cp_next_rx_seq in heap memory past the allocated slot? It would also walk
a cp_retrans list head read from that memory and call set_bit() through
whatever pointers it finds there.

If so, the commit message's claim "so a peer can never ask us to walk
lanes that do not exist" doesn't hold yet.

Could this loop also be bounded by:

    conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1

which is the count __rds_conn_create() already uses? Another option is to
skip handshake extension processing entirely on transports that are not
mp-capable.

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