Re: [PATCH] tcp: Avoid division by zero with small MSS values

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4035 bytes --]

On Wed, Oct 21, 2026 at 02:11:13AM +0200, Stefano Brivio wrote:
> On Fri, 16 Oct 2026 16:58:11 +0530
> Anshu Kumari <anskuma@redhat.com> wrote:
> 
> > Fuzz testing with AFL++ persistent mode found a crash
> 
> Yay, that's quite an achievement!
> 
> > in
> > tcp_buf_data_from_sock(), SIGFPE (signal 8) from division
> > by zero at:
> > 
> >   fill_bufs = DIV_ROUND_UP(wnd_scaled - already_sent, mss);
> > 
> > when we split buffers on MSS boundaries, if the MSS is small
> > enough (less than four, rounded down to 0).
> > 
> > A guest advertising such a small MSS indicates unusual
> > behaviour because the minimum usable MSS is
> > MTU - IP header - TCP header (28 bytes for the minimum
> > IPv4 MTU of 68). To improve robustness:
> > 
> >   - Fix MSS_SET() to round up when MSS is between 1 and 3,
> >     storing tap_mss as 1 (recovered as 4 by MSS_GET()).
> 
> This introduces a subtle problem though: if the MSS is between 1 and 3
> (silly but I guess legitimate), we would overflow it now.
> 
> I think we should rather adjust MSS_GET() as well, to always use 1 for
> values 0 < x < 4.
> 
> >   - In case of zero MSS, don't send data: tcp_buf_data_from_sock()
> >     now returns early.
> > 
> > This is not a security issue: the effect of a zero MSS from
> > a guest input was a clean process termination (SIGFPE), not
> > memory corruption or data leakage.
> 
> And, actually, a rather convenient termination because the guest might
> be doing something nasty here, so much that an assert() could also be a
> reasonable choice instead (even though I still think that trying to be
> robust against this condition makes more sense).

assert() seems dubious to me, but resetting the connection would
certainly be reasonable.  In fact, it might be best to send an RST
immediately to a SYN with MSS of 0, since such a connection would be
useless.

> Anyway, now that we continue, we should really make sure we're not
> introducing actual issues. Maybe it would be good to keep fuzzing for a
> while with this change, and see what else (if anything) a zero or very
> low MSS can actually trigger.
> 
> > 
> > Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> > ---
> >  tcp_buf.c  | 3 +++
> >  tcp_conn.h | 3 ++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tcp_buf.c b/tcp_buf.c
> > index 72c45412..ee8c7ce8 100644
> > --- a/tcp_buf.c
> > +++ b/tcp_buf.c
> > @@ -335,6 +335,9 @@ int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn,
> >  	struct iovec *iov;
> >  	uint32_t seq;
> >  
> > +	if (!mss)
> > +		return 0;
> > +
> >  	/* Set up buffer descriptors we'll fill completely and partially. */
> >  	fill_bufs = DIV_ROUND_UP(wnd_scaled - already_sent, mss);
> >  	if (fill_bufs > TCP_FRAMES) {
> > diff --git a/tcp_conn.h b/tcp_conn.h
> > index e3c55341..3f5ad28b 100644
> > --- a/tcp_conn.h
> > +++ b/tcp_conn.h
> > @@ -51,7 +51,8 @@ struct tcp_tap_conn {
> >  
> >  #define TCP_MSS_BITS			14
> >  	unsigned int	tap_mss		:TCP_MSS_BITS;
> > -#define MSS_SET(conn, mss)	(conn->tap_mss = (mss >> (16 - TCP_MSS_BITS)))
> > +#define MSS_SET(conn, mss)						\
> > +	(conn->tap_mss = (mss) ? MAX(1, (mss) >> (16 - TCP_MSS_BITS)) : 0)
> 
> I think this would deserve a comment at this point (above the define),
> clearly explaining that we are use 1 just to avoid rounding to zero for
> values less than 4.
> 
> And, I guess (untested):
> 
> >  #define MSS_GET(conn)		(conn->tap_mss << (16 - TCP_MSS_BITS))
> 
> #define MSS_GET(conn)							\
> 	((conn->tap_mss == 1) ? 1 : (conn->tap_mss << (16 - TCP_MSS_BITS)))
> 
> ...with another comment explaining that tap_mss is stored as 1 for
> any non-zero MSS value <= 4.
> 
> >  
> >  #define RTT_EXP_BITS			4
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]