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

[Allison Henderson <achender@kernel.org>]

On Thu, 2026-10-08 at 18:19 +0530, Shubham Antil wrote:

Hi Shubham,

Thanks for the v3.  Some comments below:
 
> rds_ib_process_recv() accepts an RDS/IB fragment once the receive
> completion reports at least an RDS header, and then trusts the
> header-declared message length h_len: rds_ib_inc_copy_to_user() later
> copies up to h_len bytes from the fragment pages to userspace on
> recvmsg().
> 
> When a fragment carries fewer payload bytes than h_len accounts for, the
> bytes between the end of the received payload and h_len are read from the
> unwritten tail of the fragment page.  That page comes from the per-CPU
> receive cache and is not zeroed, so recvmsg() returns stale page contents
> to userspace.
> 
> Zero the unwritten tail of a short fragment page on receipt, so the copy
> can only ever return received payload or zeros.  Fragments that fill the
> page are unaffected.
> 
> Confirmed under KMSAN: without this change recvmsg() returns stale
> frag-page bytes and KMSAN flags an uninitialised copy in
> rds_ib_inc_copy_to_user(); with it the same over-read returns zeros and
> KMSAN is clean.
> 
> Fixes: 1e23b3ee0e94 ("RDS/IB: Receive datagrams via IB")
> Reported-by: sungbyeongchan <tjdqudcks0424@naver.com>
> Closes: https://lore.kernel.org/netdev/20261006205204.1322102-1-tjdqudcks0424@naver.com/
> Reported-by: Shubham Antil <shubham@octane.security>
nit: since youre the author, you wouldn't add yourself as the reporter, so this tag should be trimmed out.

> Reported-by: Giovanni Vignone <gio@octane.security>
> Reported-by: Robert van Eijk <robert@octane.security>
> Reported-by: Paolo Gentry <paolo@octane.security>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Shubham Antil <shubham@octane.security>
> ---
> v3:
>  - Zero the unwritten fragment tail instead of dropping the connection.
>    A zero-copy send with a small iovec segment can legitimately produce a
>    non-final fragment shorter than RDS_FRAG_SIZE, so the v2 length check
>    could force a reconnect on well-formed traffic; zeroing the tail avoids
>    that while still keeping uninitialised bytes out of recvmsg().
rds_sendmsg() rejects MSG_ZEROCOPY for any transport other than TCP, so I dont
think this can happen over IB.

>    (Raised by the Sashiko review.)
I wasnt able to find the review that raised this, but the review for v2 landed after v3
was sent.  From your change log, it sounds like it was a different review, but the v2
findings are still valid, so those should be addressed:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007062135.49946-1-shubham%40octane.security

>  - Drop Reviewed-by, since the approach changed.
> v2:
>  - Add Reported-by (sungbyeongchan, first to report, and the Octane
>    Security reporters).
>  net/rds/ib_recv.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
> index bd6cb3ffa..9695c7f24 100644
> --- a/net/rds/ib_recv.c
> +++ b/net/rds/ib_recv.c
> @@ -35,6 +35,7 @@
>  #include <linux/slab.h>
>  #include <linux/pci.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/highmem.h>
>  #include <rdma/rdma_cm.h>
>  
>  #include "rds_single_path.h"
> @@ -949,6 +950,19 @@ static void rds_ib_process_recv(struct rds_connection *conn,
>  		}
>  	}
>  
> +	/* 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);
I think padding with zeros would deliver a malformed message to the application as
if it were valid. A short fragment is a protocol violation, and resetting the connection
is what we already do for a fragment header mismatch.

I notice the dashboard says a review is still queued, so I'd recommend waiting for the
v3 review and then address both in a v4.

Thanks for working on this!
Allison

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