* [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes
[not found] <cover.1780979031.git.bronzed_45_vested@icloud.com>
@ 2026-06-10 16:18 ` Ren Wei
2026-06-10 17:04 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Ren Wei @ 2026-06-10 16:18 UTC (permalink / raw)
To: netdev
Cc: edumazet, pabeni, kuba, chia-yu.chang, ij, fmancera, yuuchihsu,
idosch, herbert, yuantan098, zcliangcn, bird, bronzed_45_vested,
n05ec
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]`.
Apply the matching cross-check to `tcp_max_reordering` so the existing
invariant `tcp_max_reordering >= tcp_reordering` is preserved. This
keeps the fix at the sysctl boundary and avoids changing the TCP fast
path.
Fixes: 91cc17c0e5e5 ("[TCP]: MTUprobe: receiver window & data available checks fixed")
Cc: stable@vger.kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Zhengchuan Liang <zcliangcn@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Assisted-by: Codex:GPT-5.4
Signed-off-by: Wyatt Feng <bronzed_45_vested@icloud.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
net/ipv4/sysctl_net_ipv4.c | 38 ++++++++++++++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index c0e85cc171ae..c44afb9c321d 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -215,6 +215,40 @@ static int ipv4_fwd_update_priority(const struct ctl_table *table, int write,
return ret;
}
+static int proc_tcp_reordering(const struct ctl_table *table, int write,
+ void *buffer, size_t *lenp, loff_t *ppos)
+{
+ struct ctl_table tmp = {
+ .data = table->data,
+ .maxlen = table->maxlen,
+ .mode = table->mode,
+ .extra1 = SYSCTL_ONE,
+ };
+ struct net *net;
+
+ net = container_of(table->data, struct net, ipv4.sysctl_tcp_reordering);
+ tmp.extra2 = &net->ipv4.sysctl_tcp_max_reordering;
+
+ return proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+}
+
+static int proc_tcp_max_reordering(const struct ctl_table *table, int write,
+ void *buffer, size_t *lenp, loff_t *ppos)
+{
+ struct ctl_table tmp = {
+ .data = table->data,
+ .maxlen = table->maxlen,
+ .mode = table->mode,
+ };
+ struct net *net;
+
+ net = container_of(table->data, struct net,
+ ipv4.sysctl_tcp_max_reordering);
+ tmp.extra1 = &net->ipv4.sysctl_tcp_reordering;
+
+ return proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+}
+
static int proc_tcp_congestion_control(const struct ctl_table *ctl, int write,
void *buffer, size_t *lenp, loff_t *ppos)
{
@@ -1058,7 +1092,7 @@ static struct ctl_table ipv4_net_table[] = {
.data = &init_net.ipv4.sysctl_tcp_reordering,
.maxlen = sizeof(int),
.mode = 0644,
- .proc_handler = proc_dointvec
+ .proc_handler = proc_tcp_reordering
},
{
.procname = "tcp_retries1",
@@ -1293,7 +1327,7 @@ 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_tcp_max_reordering
},
{
.procname = "tcp_dsack",
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes
2026-06-10 16:18 ` [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes Ren Wei
@ 2026-06-10 17:04 ` Eric Dumazet
2026-06-11 9:21 ` David Laight
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2026-06-10 17:04 UTC (permalink / raw)
To: Ren Wei, Neal Cardwell, Kuniyuki Iwashima
Cc: netdev, pabeni, kuba, chia-yu.chang, ij, fmancera, yuuchihsu,
idosch, herbert, yuantan098, zcliangcn, bird, bronzed_45_vested
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]`.
>
> Apply the matching cross-check to `tcp_max_reordering` so the existing
> invariant `tcp_max_reordering >= tcp_reordering` is preserved. This
> keeps the fix at the sysctl boundary and avoids changing the TCP fast
> path.
>
> Fixes: 91cc17c0e5e5 ("[TCP]: MTUprobe: receiver window & data available checks fixed")
> Cc: stable@vger.kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Zhengchuan Liang <zcliangcn@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Assisted-by: Codex:GPT-5.4
> Signed-off-by: Wyatt Feng <bronzed_45_vested@icloud.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
> net/ipv4/sysctl_net_ipv4.c | 38 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index c0e85cc171ae..c44afb9c321d 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -215,6 +215,40 @@ static int ipv4_fwd_update_priority(const struct ctl_table *table, int write,
> return ret;
> }
>
> +static int proc_tcp_reordering(const struct ctl_table *table, int write,
> + void *buffer, size_t *lenp, loff_t *ppos)
> +{
> + struct ctl_table tmp = {
> + .data = table->data,
> + .maxlen = table->maxlen,
> + .mode = table->mode,
> + .extra1 = SYSCTL_ONE,
> + };
> + struct net *net;
> +
> + net = container_of(table->data, struct net, ipv4.sysctl_tcp_reordering);
> + tmp.extra2 = &net->ipv4.sysctl_tcp_max_reordering;
> +
> + return proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> +}
> +
> +static int proc_tcp_max_reordering(const struct ctl_table *table, int write,
> + void *buffer, size_t *lenp, loff_t *ppos)
> +{
> + struct ctl_table tmp = {
> + .data = table->data,
> + .maxlen = table->maxlen,
> + .mode = table->mode,
> + };
> + struct net *net;
> +
> + net = container_of(table->data, struct net,
> + ipv4.sysctl_tcp_max_reordering);
> + tmp.extra1 = &net->ipv4.sysctl_tcp_reordering;
> +
> + return proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> +}
> +
> static int proc_tcp_congestion_control(const struct ctl_table *ctl, int write,
> void *buffer, size_t *lenp, loff_t *ppos)
> {
> @@ -1058,7 +1092,7 @@ static struct ctl_table ipv4_net_table[] = {
> .data = &init_net.ipv4.sysctl_tcp_reordering,
> .maxlen = sizeof(int),
> .mode = 0644,
> - .proc_handler = proc_dointvec
> + .proc_handler = proc_tcp_reordering
> },
> {
> .procname = "tcp_retries1",
> @@ -1293,7 +1327,7 @@ 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_tcp_max_reordering
> },
> {
> .procname = "tcp_dsack",
> --
> 2.47.3
>
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,
},
{
.procname = "tcp_dsack",
@@ -1676,6 +1680,9 @@ static __net_init int
ipv4_sysctl_init_net(struct net *net)
*/
table[i].mode &= ~0222;
}
+ if (table[i].extra2 >= (void *)&init_net.ipv4 &&
+ table[i].extra2 < (void *)(&init_net.ipv4 + 1))
+ table[i].extra2 += (void *)net - (void *)&init_net;
}
}
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 6e4bb411dc041b2a299f0fbc2109c530afa2a345..193637a58dcc1acf8e70fdfbde982c2a3eed290e
100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2687,7 +2687,7 @@ static int tcp_mtu_probe(struct sock *sk)
struct sk_buff *skb, *nskb, *next;
struct net *net = sock_net(sk);
int probe_size;
- int size_needed;
+ u64 size_needed;
int copy, len;
int mss_now;
int interval;
@@ -2711,7 +2711,7 @@ static int tcp_mtu_probe(struct sock *sk)
mss_now = tcp_current_mss(sk);
probe_size = tcp_mtu_to_mss(sk, (icsk->icsk_mtup.search_high +
icsk->icsk_mtup.search_low) >> 1);
- size_needed = probe_size + (tp->reordering + 1) * tp->mss_cache;
+ size_needed = probe_size + (tp->reordering + 1) * (u64)tp->mss_cache;
interval = icsk->icsk_mtup.search_high - icsk->icsk_mtup.search_low;
/* When misfortune happens, we are reprobing actively,
* and then reprobe timer has expired. We stick with current
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes
2026-06-10 17:04 ` Eric Dumazet
@ 2026-06-11 9:21 ` David Laight
2026-06-11 17:13 ` Eric Dumazet
2026-06-11 17:18 ` Eric Dumazet
0 siblings, 2 replies; 6+ messages in thread
From: David Laight @ 2026-06-11 9:21 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ren Wei, Neal Cardwell, Kuniyuki Iwashima, netdev, pabeni, kuba,
chia-yu.chang, ij, fmancera, yuuchihsu, idosch, herbert,
yuantan098, zcliangcn, bird, bronzed_45_vested
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes
2026-06-11 9:21 ` David Laight
@ 2026-06-11 17:13 ` Eric Dumazet
2026-06-12 2:01 ` Kuniyuki Iwashima
2026-06-11 17:18 ` Eric Dumazet
1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2026-06-11 17:13 UTC (permalink / raw)
To: David Laight
Cc: Ren Wei, Neal Cardwell, Kuniyuki Iwashima, netdev, pabeni, kuba,
chia-yu.chang, ij, fmancera, yuuchihsu, idosch, herbert,
yuantan098, zcliangcn, bird, bronzed_45_vested
On Thu, Jun 11, 2026 at 2:21 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> 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).
Kuniyuki, can you please send a fix ? You used u8 in your commit :/
Default value of this sysctl is 3, but it could potentially exceed 255.
Thanks.
commit 46778cd16e6a5ad1b2e3a91f6c057c907379418e
Author: Kuniyuki Iwashima <kuniyu@google.com>
Date: Fri Jul 15 10:17:49 2022 -0700
tcp: Fix data-races around sysctl_tcp_reordering.
While reading sysctl_tcp_reordering, it can be changed concurrently.
Thus, we need to add READ_ONCE() to its readers.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes
2026-06-11 9:21 ` David Laight
2026-06-11 17:13 ` Eric Dumazet
@ 2026-06-11 17:18 ` Eric Dumazet
1 sibling, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2026-06-11 17:18 UTC (permalink / raw)
To: David Laight
Cc: Ren Wei, Neal Cardwell, Kuniyuki Iwashima, netdev, pabeni, kuba,
chia-yu.chang, ij, fmancera, yuuchihsu, idosch, herbert,
yuantan098, zcliangcn, bird, bronzed_45_vested
On Thu, Jun 11, 2026 at 2:21 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> 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?
Probably, I am not even sure we use this much with RACK anyway.
Neal, what do you think?
>
> 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes
2026-06-11 17:13 ` Eric Dumazet
@ 2026-06-12 2:01 ` Kuniyuki Iwashima
0 siblings, 0 replies; 6+ messages in thread
From: Kuniyuki Iwashima @ 2026-06-12 2:01 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Laight, Ren Wei, Neal Cardwell, netdev, pabeni, kuba,
chia-yu.chang, ij, fmancera, yuuchihsu, idosch, herbert,
yuantan098, zcliangcn, bird, bronzed_45_vested
On Thu, Jun 11, 2026 at 10:14 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Jun 11, 2026 at 2:21 AM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > 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).
>
>
> Kuniyuki, can you please send a fix ? You used u8 in your commit :/
Oh sorry, I'll change it to int.
>
> Default value of this sysctl is 3, but it could potentially exceed 255.
>
> Thanks.
>
> commit 46778cd16e6a5ad1b2e3a91f6c057c907379418e
> Author: Kuniyuki Iwashima <kuniyu@google.com>
> Date: Fri Jul 15 10:17:49 2022 -0700
>
> tcp: Fix data-races around sysctl_tcp_reordering.
>
> While reading sysctl_tcp_reordering, it can be changed concurrently.
> Thus, we need to add READ_ONCE() to its readers.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-06-12 2:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1780979031.git.bronzed_45_vested@icloud.com>
2026-06-10 16:18 ` [PATCH net 1/1] net: ipv4: bound TCP reordering sysctl writes Ren Wei
2026-06-10 17:04 ` Eric Dumazet
2026-06-11 9:21 ` David Laight
2026-06-11 17:13 ` Eric Dumazet
2026-06-12 2:01 ` Kuniyuki Iwashima
2026-06-11 17:18 ` Eric Dumazet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.