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

[Stefano Brivio <sbrivio@redhat.com>]

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).

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