Re: [rds-devel] [External] : Re: [PATCH net] RDS/IB: validate receive completion payload length

[Allison Henderson <achender@kernel.org>]

On Fri, 2026-10-16 at 13:18 -0700, Gerd Rausch wrote:
> Hi,
> 
> On 2026-10-06 18:13, Allison Henderson via rds-devel wrote:
> > > --- a/net/rds/ib_recv.c
> > > +++ b/net/rds/ib_recv.c
> > > @@ -949,6 +949,15 @@ static void rds_ib_process_recv(struct rds_connection *conn,
> > >  		}
> > >  	}
> > >  
> > > +	if (data_len != min_t(u32, ic->i_recv_data_rem, RDS_FRAG_SIZE)) {
> > > +		rds_ib_conn_error(conn,
> > > +				  "incoming fragment payload length %u, expected %u; "
> > > +				  "disconnecting and reconnecting\n",
> > > +				  data_len,
> > > +				  min_t(u32, ic->i_recv_data_rem, RDS_FRAG_SIZE));
> > > +		goto done;
> > > +	}
> > > +
> > >  	list_add_tail(&recv->r_frag->f_item, &ibinc->ii_frags);
> > >  	recv->r_frag = NULL;
> > >  
> How does forcing a reconnect help this situation?
> 
> After the reconnect, the sender submits the exact same (incorrect) message again,
> which it kept in "cp_retrans".
> 
> Therefore, the exact same conditional triggers again.
> 
> All but the last fragment of an RDS/IB message are expected to be of RDS_FRAG_SIZE.
> 
> While there's no good protocol specification (to the best of my knowledge),
> posting an insufficiently large message can be seen as protocol violation.
> 

Hi Gerd,

First, some bookkeeping to catch up with: this patch was submitted
slightly after Shubham's "RDS/IB: validate receive fragment completions",
which effectively the same patch.  Shubham's variant went through a few
revisions on netdev and is now in the net tree as 65cf6863606b.  It
varies only slightly, so your questions still apply, but to a different
patch and one that's already merged:

https://lore.kernel.org/netdev/20261013025729.40815-1-shubham@octane.security/

On the retransmit loop: cp_retrans only matters if the sender is our
own RDS stack, and our stack can't produce the fragment this check
rejects.  rds_ib_xmit() posts every fragment but the last at
RDS_FRAG_SIZE, because the scatterlist entries it works from are full
pages except the final one.  It even BUG_ONs if it's ever asked to
resume a message at an offset that isn't a multiple of RDS_FRAG_SIZE,
which can only hold if everything sent so far went out in full
fragments.
  
MSG_ZEROCOPY is the only path that produces odd-sized entries, and
rds_sendmsg() refuses it for anything but TCP.  So a Linux peer never
trips this, before or after a reconnect.  The peer that does trip it
is already off the protocol, as you say, and whatever it retransmits
isn't governed by our cp_retrans.

> Therefore, apart from security concerns, the previous behavior of garbage-in / garbage-out
> is IMO better than this behavior of just endlessly forcing a reconnect.

The trouble is that the garbage going out wasn't the sender's.  The
receiver delivered h_len bytes of a page that only had data_len bytes
written into it, so the tail was whatever the page held before: an
earlier message to another socket (the report demonstrated this across
UIDs), or unzeroed memory on a fresh page.  That's the bug. And
refusing to deliver it is the whole fix.

Dropping the connection is also what rds_ib_process_recv() has always
done for the neighbouring case, a fragment whose header doesn't match
the message in progress.  That check has been there since 1e23b3ee0e94
and has the same "a misbehaving peer can keep us reconnecting"
property.  This patch doesn't add a new failure mode, it just closes
the one way a malformed fragment could still get through.
> 

> If there's a strong desire to address this issue, the sender should be
> told to discard the message. A reconnect doesn't address this issue.
> 
I don't think there's a way to say that today.  The RDS header flags are
congestion-bitmap, ack-required, retransmitted and the extension-header
bit.  The only error a receiver can signal to its peer is to drop the
connection.

A per-message reject would be a protocol extension that both ends have to
implement.  But that would be net-next material, and it couldn't help
against the only peers that actually send these.  I'd be happy to review it
if someone wants to take it on, but I don't think it should hold up a stable
fix for a data disclosure.

Hope that helps!
Allison

> Thanks,
> 
>    Gerd
>