Re: [PATCH net v3] net/rds: ib: zero the unwritten tail of a short receive fragment

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rds: ib: zeroing short non-final fragments corrupts payload and desynchronizes framing
- [High] net/rds: unbounded memory allocation on receive via untrusted h_len
--

commit 155e0b4d269458cc1b33d64be6029fcc1b1efa50
Author: Shubham Antil <shubham@octane.security>
Subject: net/rds: ib: zero the unwritten tail of a short receive fragment

This commit zeroes the unwritten tail of short receive fragments to prevent
stale page contents from being copied to userspace. It attempts to resolve a
KMSAN uninitialized memory warning by ensuring the tail of the fragment is
zeroed before processing.

Closes: https://lore.kernel.org/netdev/20261006205204.1322102-1-tjdqudcks0424@naver.com/

> diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
> index bd6cb3ffaa571..9695c7f24eaa7 100644
> --- a/net/rds/ib_recv.c
> +++ b/net/rds/ib_recv.c

[ ... ]

> @@ -949,6 +950,19 @@ static void rds_ib_process_recv(struct rds_connection *conn,
>  		}
>  	}
>  

[Severity: High]
Does this code allow unbounded memory allocation based on an untrusted header
field?

Earlier in rds_ib_process_recv(), the receiver extracts hdr->h_len from the
network and initializes ic->i_recv_data_rem:

net/rds/ib_recv.c:rds_ib_process_recv() {
    ...
    memcpy(hdr, ihdr, sizeof(*hdr));
    ic->i_recv_data_rem = be32_to_cpu(hdr->h_len);
    ...
}

It doesn't appear to validate it against RDS_MAX_MSG_SIZE. Can a malicious
remote peer stream data with a massive h_len to consume gigabytes of memory
per message and cause system exhaustion (OOM), especially since the same
unbounded h_len behavior appears to exist in TCP receive via
rds_tcp_data_recv()?

net/rds/tcp_recv.c:rds_tcp_data_recv() {
    ...
    if (tc->t_tinc_hdr_rem == 0) {
        /* could be 0 for a 0 len message */
        tc->t_tinc_data_rem =
            be32_to_cpu(tinc->ti_inc.i_hdr.h_len);
    ...
}

> +	/* The device wrote only data_len bytes into this recycled fragment
> +	 * page.  Zero the unwritten tail so a later rds_ib_inc_copy_to_user(),
> +	 * which copies up to the peer-declared h_len, cannot hand stale page
> +	 * contents to userspace.
> +	 */
> +	if (data_len < RDS_FRAG_SIZE) {
> +		void *frag = kmap_local_page(sg_page(&recv->r_frag->f_sg));
> +
> +		memset(frag + recv->r_frag->f_sg.offset + data_len, 0,
> +		       RDS_FRAG_SIZE - data_len);

[Severity: High]
Can zeroing the unwritten tail here cause silent data corruption and
desynchronize the protocol framing?

The commit message explicitly states that a zero-copy send with a small iovec
segment can legitimately produce a non-final fragment shorter than
RDS_FRAG_SIZE. However, the receiver's framing logic statically assumes every
non-final fragment contains exactly RDS_FRAG_SIZE bytes:

net/rds/ib_recv.c:rds_ib_process_recv() {
    ...
    if (ic->i_recv_data_rem > RDS_FRAG_SIZE)
        ic->i_recv_data_rem -= RDS_FRAG_SIZE;
    ...
}

And the copy routine blindly copies up to RDS_FRAG_SIZE bytes from each
fragment without checking the actual received data_len:

net/rds/ib_recv.c:rds_ib_inc_copy_to_user() {
    ...
    to_copy = min_t(unsigned long, iov_iter_count(to),
                    RDS_FRAG_SIZE - frag_off);
    to_copy = min_t(unsigned long, to_copy, len - copied);
    ...
}

Doesn't this mean the zeroes injected by the patch are copied to userspace as
valid payload bytes, shifting and truncating the actual data instead of
correctly handling short non-final fragments?

> +		kunmap_local(frag);
> +	}
> +
>  	list_add_tail(&recv->r_frag->f_item, &ibinc->ii_frags);
>  	recv->r_frag = NULL;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008124940.13371-1-shubham@octane.security?part=1