diff mbox series

[v2,1/1] net: sysctl: Bound check gc_thresh sysctls

Message ID 20250128103821.29745-1-nicolas.bouchinet@clip-os.org (mailing list archive)
State Rejected
Delegated to: Netdev Maintainers
Headers show
Series [v2,1/1] net: sysctl: Bound check gc_thresh sysctls | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 5 this patch: 5
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 30 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2025-01-28--15-00 (tests: 884)

Commit Message

Nicolas Bouchinet Jan. 28, 2025, 10:38 a.m. UTC 
From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

ipv4, ipv6 and xfrm6 gc_thresh sysctls were authorized to be written any
negative values, which would be stored in an unsigned int backing data
(the gc_thresh variable in the dst_ops struct) since the proc_handler
was proc_dointvec.

It seems to be used to disables garbage collection of
`net/ipv4/route/gc_thresh` since commit: 4ff3885262d0 ("ipv4: Delete
routing cache."). gc_thresh variable being set to `~0`.

To clarify the sysctl interface, the proc_handler has thus been updated
to proc_dointvec_minmax and writings have between limited between
SYSCTL_NEG_ONE and SYSCTL_INT_MAX.

With this patch applied, sysctl writes outside the defined in the bound
will thus lead to a write error :

```
echo "-2" > /proc/sys/net/ipv4/route/gc_thresh
bash: echo: write error: Invalid argument
```

Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

---

Changes since v1:
https://lore.kernel.org/all/20250127142014.37834-1-nicolas.bouchinet@clip-os.org/

* Detatched the patch from the patchset
* Updated the boundcheck between SYSCTL_NEG_ONE and SYSCTL_INT_MAX.
* Reworded the commit message to make it more clear.

---
 net/ipv4/route.c        | 4 +++-
 net/ipv6/route.c        | 4 +++-
 net/ipv6/xfrm6_policy.c | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

Comments

Eric Dumazet Jan. 28, 2025, 1:50 p.m. UTC | #1 
On Tue, Jan 28, 2025 at 11:39 AM <nicolas.bouchinet@clip-os.org> wrote:
>
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
>
> ipv4, ipv6 and xfrm6 gc_thresh sysctls were authorized to be written any
> negative values, which would be stored in an unsigned int backing data
> (the gc_thresh variable in the dst_ops struct) since the proc_handler
> was proc_dointvec.
>
> It seems to be used to disables garbage collection of
> `net/ipv4/route/gc_thresh` since commit: 4ff3885262d0 ("ipv4: Delete
> routing cache."). gc_thresh variable being set to `~0`.

Simply reflecting this sysctl was kept to avoid user scripts to fail.

Kernel ignores its value.

>
> To clarify the sysctl interface, the proc_handler has thus been updated
> to proc_dointvec_minmax and writings have between limited between
> SYSCTL_NEG_ONE and SYSCTL_INT_MAX.
>
> With this patch applied, sysctl writes outside the defined in the bound
> will thus lead to a write error :
>
> ```
> echo "-2" > /proc/sys/net/ipv4/route/gc_thresh
> bash: echo: write error: Invalid argument
> ```
>
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

gc_thresh is "unsigned int"

Why would we allow setting it to 0xFFFFFFFF but not 0xCF012345 ?

Your patch seems not needed to me.
diff mbox series

Patch

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 0fbec35096186..96641ae15049a 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -3453,7 +3453,9 @@  static struct ctl_table ipv4_route_table[] = {
 		.data		= &ipv4_dst_ops.gc_thresh,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_NEG_ONE,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
 		.procname	= "max_size",
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 67ff16c047180..3fc7f336dfa04 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -6379,7 +6379,9 @@  static struct ctl_table ipv6_route_table_template[] = {
 		.data		=	&ip6_dst_ops_template.gc_thresh,
 		.maxlen		=	sizeof(int),
 		.mode		=	0644,
-		.proc_handler	=	proc_dointvec,
+		.proc_handler	=	proc_dointvec_minmax,
+		.extra1		=	SYSCTL_NEG_ONE,
+		.extra2		=	SYSCTL_INT_MAX,
 	},
 	{
 		.procname	=	"flush",
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index 1f19b6f14484c..1e212d5341839 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -189,7 +189,9 @@  static struct ctl_table xfrm6_policy_table[] = {
 		.data		= &init_net.xfrm.xfrm6_dst_ops.gc_thresh,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler   = proc_dointvec,
+		.proc_handler   = proc_dointvec_minmax,
+		.extra1		= SYSCTL_NEG_ONE,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 };