Re: [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes

[David Laight <david.laight.linux@gmail.com>]

On Wed, 10 Jun 2026 10:04:06 -0700
Eric Dumazet <edumazet@google.com> wrote:

> On Wed, Jun 10, 2026 at 9:18 AM Ren Wei <n05ec@lzu.edu.cn> wrote:
> >
> > From: Wyatt Feng <bronzed_45_vested@icloud.com>
> >
> > Reject invalid `net.ipv4.tcp_reordering` values before they reach TCP
> > socket state. The sysctl is stored as an `int` but copied into the
> > `u32` `tp->reordering` field for new sockets, so negative writes wrap
> > to large values.
> >
> > With `tcp_mtu_probing=2`, the wrapped value can overflow the
> > `tcp_mtu_probe()` size calculation and drive the MTU probing path into
> > an out-of-bounds read. Route `tcp_reordering` writes through
> > `proc_dointvec_minmax()` and clamp them to the per-netns range
> > `[1, tcp_max_reordering]`.
> >
...
> Thanks for the patch. I think we can do better.
> 
> Also I do not see a fix in tcp_mtu_probe()?
> 
> What about:
> 
> 
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index c0e85cc171aec099fd5d4897b1a623dd27eaee08..987bc87e255a81680ae5e23ddd01c1f0de4425ec
> 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -1058,6 +1058,9 @@ static struct ctl_table ipv4_net_table[] = {
>                 .data           = &init_net.ipv4.sysctl_tcp_reordering,
>                 .maxlen         = sizeof(int),
>                 .mode           = 0644,
> +               .proc_handler   = proc_dointvec_minmax,
> +               .extra1         = SYSCTL_ONE,
> +               .extra2         = &init_net.ipv4.sysctl_tcp_max_reordering,
>                 .proc_handler   = proc_dointvec
>         },
>         {
> @@ -1293,7 +1296,8 @@ static struct ctl_table ipv4_net_table[] = {
>                 .data           = &init_net.ipv4.sysctl_tcp_max_reordering,
>                 .maxlen         = sizeof(int),
>                 .mode           = 0644,
> -               .proc_handler   = proc_dointvec
> +               .proc_handler   = proc_dointvec_minmax,
> +               .extra1         = SYSCTL_ONE,

Would it be reasonable to put in a 'sanity' upper bound here?

Since tcp_max_reordering can be lowered after tcp_reordering is set
the bound set here isn't 'hard'.
So the same sanity limit for both may be enough?

I also found this 'gem':
void tcp_enter_loss(struct sock *sk)
{
...
	u8 reordering;
...
	reordering = READ_ONCE(net->ipv4.sysctl_tcp_reordering);

Which makes be think that someone expected these values to be small.

I don't know what the sane bounds are for either sysctl or tp->reordering
itself (which is u32).

-- David