[PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[~akihirosuda]

From: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

With this commit, all the GIDs ("0 4294967294") can be written to the
"net.ipv4.ping_group_range" sysctl.

Note that 4294967295 (0xffffffff) is an invalid GID (see gid_valid() in
include/linux/uidgid.h), and an attempt to register this number will cause
-EINVAL.

Prior to this commit, only up to GID 2147483647 could be covered.
Documentation/networking/ip-sysctl.rst had "0 4294967295" as an example
value, but this example was wrong and causing -EINVAL.

In the implementation, proc_dointvec_minmax is no longer used because it
does not support numbers from 2147483648 to 4294967294.

proc_douintvec is not used either, because it does not support
multi-element vectors, despite its function name.
Commit 4f2fec00afa6 ("sysctl: simplify unsigned int support") says
"*Do not* add support for them".

Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
---
 Documentation/networking/ip-sysctl.rst |  4 +-
 include/net/ping.h                     |  6 ---
 net/ipv4/sysctl_net_ipv4.c             | 52 +++++++++++++++++++++-----
 3 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 6ec06a33688a..80b8f73a0244 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -1352,8 +1352,8 @@ ping_group_range - 2 INTEGERS
 	Restrict ICMP_PROTO datagram sockets to users in the group range.
 	The default is "1 0", meaning, that nobody (not even root) may
 	create ping sockets.  Setting it to "100 100" would grant permissions
-	to the single group. "0 4294967295" would enable it for the world, "100
-	4294967295" would enable it for the users, but not daemons.
+	to the single group. "0 4294967294" would enable it for the world, "100
+	4294967294" would enable it for the users, but not daemons.
 
 tcp_early_demux - BOOLEAN
 	Enable early demux for established TCP sockets.
diff --git a/include/net/ping.h b/include/net/ping.h
index 9233ad3de0ad..37b1d7baeb7b 100644
--- a/include/net/ping.h
+++ b/include/net/ping.h
@@ -16,12 +16,6 @@
 #define PING_HTABLE_SIZE 	64
 #define PING_HTABLE_MASK 	(PING_HTABLE_SIZE-1)
 
-/*
- * gid_t is either uint or ushort.  We want to pass it to
- * proc_dointvec_minmax(), so it must not be larger than MAX_INT
- */
-#define GID_T_MAX (((gid_t)~0U) >> 1)
-
 /* Compatibility glue so we can support IPv6 when it's compiled as a module */
 struct pingv6_ops {
 	int (*ipv6_recv_error)(struct sock *sk, struct msghdr *msg, int len,
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 40fe70fc2015..5f077156ec50 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -34,8 +34,6 @@ static int ip_ttl_min = 1;
 static int ip_ttl_max = 255;
 static int tcp_syn_retries_min = 1;
 static int tcp_syn_retries_max = MAX_TCP_SYNCNT;
-static int ip_ping_group_range_min[] = { 0, 0 };
-static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
 static u32 u32_max_div_HZ = UINT_MAX / HZ;
 static int one_day_secs = 24 * 3600;
 static u32 fib_multipath_hash_fields_all_mask __maybe_unused =
@@ -167,24 +165,56 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
 	int ret;
 	gid_t urange[2];
 	kgid_t low, high;
+	size_t slen = 256; /* total bytes including '\0' */
+	char *s = kmalloc(slen, GFP_KERNEL); /* clobbered by strsep */
 	struct ctl_table tmp = {
-		.data = &urange,
-		.maxlen = sizeof(urange),
+		.data = s,
+		.maxlen = slen,
 		.mode = table->mode,
-		.extra1 = &ip_ping_group_range_min,
-		.extra2 = &ip_ping_group_range_max,
 	};
 
+	if (unlikely(!s))
+		return -ENOMEM;
+
 	inet_get_ping_group_range_table(table, &low, &high);
 	urange[0] = from_kgid_munged(user_ns, low);
 	urange[1] = from_kgid_munged(user_ns, high);
-	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
-
+	/* proc_dointvec_minmax is no longer used because it does not support
+	 * numbers from 2147483648 to 4294967294.
+	 *
+	 * proc_douintvec is not used either, because it does not support
+	 * multi-element vectors, despite its function name.
+	 * Commit 4f2fec00afa6 ("sysctl: simplify unsigned int support") says
+	 * "*Do not* add support for them".
+	 */
+	ret = snprintf(tmp.data, slen, "%u\t%u", urange[0], urange[1]);
+	if (ret < 0)
+		goto done;
+	ret = proc_dostring(&tmp, write, buffer, lenp, ppos);
+	if (*lenp >= slen - 1) /* truncated */
+		ret = -EINVAL;
 	if (write && ret == 0) {
+		char *tok[2];
+		int i;
+
+		s = strim(s);
+		tok[0] = strsep(&s, " \t");
+		tok[1] = s;
+		for (i = 0; i < 2; i++) {
+			if (!tok[i]) {
+				ret = -EINVAL;
+				goto done;
+			}
+			ret = kstrtouint(tok[i], 0, &urange[i]);
+			if (ret < 0)
+				goto done;
+		}
 		low = make_kgid(user_ns, urange[0]);
 		high = make_kgid(user_ns, urange[1]);
-		if (!gid_valid(low) || !gid_valid(high))
-			return -EINVAL;
+		if (!gid_valid(low) || !gid_valid(high)) {
+			ret = -EINVAL;
+			goto done;
+		}
 		if (urange[1] < urange[0] || gid_lt(high, low)) {
 			low = make_kgid(&init_user_ns, 1);
 			high = make_kgid(&init_user_ns, 0);
@@ -192,6 +222,8 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
 		set_ping_group_range(table, low, high);
 	}
 
+done:
+	kfree(tmp.data);
 	return ret;
 }
 
-- 
2.38.4

Re: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[Kuniyuki Iwashima]

From: ~akihirosuda <akihirosuda@git.sr.ht>
Date: Wed, 31 May 2023 19:42:49 +0900
> From: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
> 
> With this commit, all the GIDs ("0 4294967294") can be written to the
> "net.ipv4.ping_group_range" sysctl.
> 
> Note that 4294967295 (0xffffffff) is an invalid GID (see gid_valid() in
> include/linux/uidgid.h), and an attempt to register this number will cause
> -EINVAL.
> 
> Prior to this commit, only up to GID 2147483647 could be covered.
> Documentation/networking/ip-sysctl.rst had "0 4294967295" as an example
> value, but this example was wrong and causing -EINVAL.
> 
> In the implementation, proc_dointvec_minmax is no longer used because it
> does not support numbers from 2147483648 to 4294967294.

Good catch.

I think we can use proc_doulongvec_minmax() instead of open coding.

With the diff below:

---8<---
# sysctl -a | grep ping
net.ipv4.ping_group_range = 0	2147483647
# sysctl -w net.ipv4.ping_group_range="0 4294967295"
sysctl: setting key "net.ipv4.ping_group_range": Invalid argument
# sysctl -w net.ipv4.ping_group_range="0 4294967294"
net.ipv4.ping_group_range = 0 4294967294
# sysctl -a | grep ping
net.ipv4.ping_group_range = 0	4294967294
---8<---

---8<---
diff --git a/include/net/ping.h b/include/net/ping.h
index 9233ad3de0ad..9b401b9a9d35 100644
--- a/include/net/ping.h
+++ b/include/net/ping.h
@@ -20,7 +20,7 @@
  * gid_t is either uint or ushort.  We want to pass it to
  * proc_dointvec_minmax(), so it must not be larger than MAX_INT
  */
-#define GID_T_MAX (((gid_t)~0U) >> 1)
+#define GID_T_MAX ((gid_t)~0U)
 
 /* Compatibility glue so we can support IPv6 when it's compiled as a module */
 struct pingv6_ops {
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 6ae3345a3bdf..11d401958673 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -35,8 +35,8 @@ static int ip_ttl_max = 255;
 static int tcp_syn_retries_min = 1;
 static int tcp_syn_retries_max = MAX_TCP_SYNCNT;
 static int tcp_syn_linear_timeouts_max = MAX_TCP_SYNCNT;
-static int ip_ping_group_range_min[] = { 0, 0 };
-static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
+static unsigned long ip_ping_group_range_min[] = { 0, 0 };
+static unsigned long ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
 static u32 u32_max_div_HZ = UINT_MAX / HZ;
 static int one_day_secs = 24 * 3600;
 static u32 fib_multipath_hash_fields_all_mask __maybe_unused =
@@ -165,8 +165,8 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
 				 void *buffer, size_t *lenp, loff_t *ppos)
 {
 	struct user_namespace *user_ns = current_user_ns();
+	unsigned long urange[2];
 	int ret;
-	gid_t urange[2];
 	kgid_t low, high;
 	struct ctl_table tmp = {
 		.data = &urange,
@@ -179,7 +179,7 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
 	inet_get_ping_group_range_table(table, &low, &high);
 	urange[0] = from_kgid_munged(user_ns, low);
 	urange[1] = from_kgid_munged(user_ns, high);
-	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+	ret = proc_doulongvec_minmax(&tmp, write, buffer, lenp, ppos);
 
 	if (write && ret == 0) {
 		low = make_kgid(user_ns, urange[0]);
---8<---


> 
> proc_douintvec is not used either, because it does not support
> multi-element vectors, despite its function name.
> Commit 4f2fec00afa6 ("sysctl: simplify unsigned int support") says
> "*Do not* add support for them".
> 
> Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
> Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
> ---
>  Documentation/networking/ip-sysctl.rst |  4 +-
>  include/net/ping.h                     |  6 ---
>  net/ipv4/sysctl_net_ipv4.c             | 52 +++++++++++++++++++++-----
>  3 files changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 6ec06a33688a..80b8f73a0244 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -1352,8 +1352,8 @@ ping_group_range - 2 INTEGERS
>  	Restrict ICMP_PROTO datagram sockets to users in the group range.
>  	The default is "1 0", meaning, that nobody (not even root) may
>  	create ping sockets.  Setting it to "100 100" would grant permissions
> -	to the single group. "0 4294967295" would enable it for the world, "100
> -	4294967295" would enable it for the users, but not daemons.
> +	to the single group. "0 4294967294" would enable it for the world, "100
> +	4294967294" would enable it for the users, but not daemons.
>  
>  tcp_early_demux - BOOLEAN
>  	Enable early demux for established TCP sockets.
> diff --git a/include/net/ping.h b/include/net/ping.h
> index 9233ad3de0ad..37b1d7baeb7b 100644
> --- a/include/net/ping.h
> +++ b/include/net/ping.h
> @@ -16,12 +16,6 @@
>  #define PING_HTABLE_SIZE 	64
>  #define PING_HTABLE_MASK 	(PING_HTABLE_SIZE-1)
>  
> -/*
> - * gid_t is either uint or ushort.  We want to pass it to
> - * proc_dointvec_minmax(), so it must not be larger than MAX_INT
> - */
> -#define GID_T_MAX (((gid_t)~0U) >> 1)
> -
>  /* Compatibility glue so we can support IPv6 when it's compiled as a module */
>  struct pingv6_ops {
>  	int (*ipv6_recv_error)(struct sock *sk, struct msghdr *msg, int len,
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index 40fe70fc2015..5f077156ec50 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -34,8 +34,6 @@ static int ip_ttl_min = 1;
>  static int ip_ttl_max = 255;
>  static int tcp_syn_retries_min = 1;
>  static int tcp_syn_retries_max = MAX_TCP_SYNCNT;
> -static int ip_ping_group_range_min[] = { 0, 0 };
> -static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
>  static u32 u32_max_div_HZ = UINT_MAX / HZ;
>  static int one_day_secs = 24 * 3600;
>  static u32 fib_multipath_hash_fields_all_mask __maybe_unused =
> @@ -167,24 +165,56 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
>  	int ret;
>  	gid_t urange[2];
>  	kgid_t low, high;
> +	size_t slen = 256; /* total bytes including '\0' */
> +	char *s = kmalloc(slen, GFP_KERNEL); /* clobbered by strsep */
>  	struct ctl_table tmp = {
> -		.data = &urange,
> -		.maxlen = sizeof(urange),
> +		.data = s,
> +		.maxlen = slen,
>  		.mode = table->mode,
> -		.extra1 = &ip_ping_group_range_min,
> -		.extra2 = &ip_ping_group_range_max,
>  	};
>  
> +	if (unlikely(!s))
> +		return -ENOMEM;
> +
>  	inet_get_ping_group_range_table(table, &low, &high);
>  	urange[0] = from_kgid_munged(user_ns, low);
>  	urange[1] = from_kgid_munged(user_ns, high);
> -	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> -
> +	/* proc_dointvec_minmax is no longer used because it does not support
> +	 * numbers from 2147483648 to 4294967294.
> +	 *
> +	 * proc_douintvec is not used either, because it does not support
> +	 * multi-element vectors, despite its function name.
> +	 * Commit 4f2fec00afa6 ("sysctl: simplify unsigned int support") says
> +	 * "*Do not* add support for them".
> +	 */
> +	ret = snprintf(tmp.data, slen, "%u\t%u", urange[0], urange[1]);
> +	if (ret < 0)
> +		goto done;
> +	ret = proc_dostring(&tmp, write, buffer, lenp, ppos);
> +	if (*lenp >= slen - 1) /* truncated */
> +		ret = -EINVAL;
>  	if (write && ret == 0) {
> +		char *tok[2];
> +		int i;
> +
> +		s = strim(s);
> +		tok[0] = strsep(&s, " \t");
> +		tok[1] = s;
> +		for (i = 0; i < 2; i++) {
> +			if (!tok[i]) {
> +				ret = -EINVAL;
> +				goto done;
> +			}
> +			ret = kstrtouint(tok[i], 0, &urange[i]);
> +			if (ret < 0)
> +				goto done;
> +		}
>  		low = make_kgid(user_ns, urange[0]);
>  		high = make_kgid(user_ns, urange[1]);
> -		if (!gid_valid(low) || !gid_valid(high))
> -			return -EINVAL;
> +		if (!gid_valid(low) || !gid_valid(high)) {
> +			ret = -EINVAL;
> +			goto done;
> +		}
>  		if (urange[1] < urange[0] || gid_lt(high, low)) {
>  			low = make_kgid(&init_user_ns, 1);
>  			high = make_kgid(&init_user_ns, 0);
> @@ -192,6 +222,8 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
>  		set_ping_group_range(table, low, high);
>  	}
>  
> +done:
> +	kfree(tmp.data);
>  	return ret;
>  }
>  
> -- 
> 2.38.4

Re: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[Eric Dumazet]

On Wed, May 31, 2023 at 9:19 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: ~akihirosuda <akihirosuda@git.sr.ht>
> Date: Wed, 31 May 2023 19:42:49 +0900
> > From: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
> >
> > With this commit, all the GIDs ("0 4294967294") can be written to the
> > "net.ipv4.ping_group_range" sysctl.
> >
> > Note that 4294967295 (0xffffffff) is an invalid GID (see gid_valid() in
> > include/linux/uidgid.h), and an attempt to register this number will cause
> > -EINVAL.
> >
> > Prior to this commit, only up to GID 2147483647 could be covered.
> > Documentation/networking/ip-sysctl.rst had "0 4294967295" as an example
> > value, but this example was wrong and causing -EINVAL.
> >
> > In the implementation, proc_dointvec_minmax is no longer used because it
> > does not support numbers from 2147483648 to 4294967294.
>
> Good catch.
>
> I think we can use proc_doulongvec_minmax() instead of open coding.
>
> With the diff below:
>
> ---8<---
> # sysctl -a | grep ping
> net.ipv4.ping_group_range = 0   2147483647
> # sysctl -w net.ipv4.ping_group_range="0 4294967295"
> sysctl: setting key "net.ipv4.ping_group_range": Invalid argument
> # sysctl -w net.ipv4.ping_group_range="0 4294967294"
> net.ipv4.ping_group_range = 0 4294967294
> # sysctl -a | grep ping
> net.ipv4.ping_group_range = 0   4294967294
> ---8<---
>
> ---8<---
> diff --git a/include/net/ping.h b/include/net/ping.h
> index 9233ad3de0ad..9b401b9a9d35 100644
> --- a/include/net/ping.h
> +++ b/include/net/ping.h
> @@ -20,7 +20,7 @@
>   * gid_t is either uint or ushort.  We want to pass it to
>   * proc_dointvec_minmax(), so it must not be larger than MAX_INT
>   */
> -#define GID_T_MAX (((gid_t)~0U) >> 1)
> +#define GID_T_MAX ((gid_t)~0U)
>
>  /* Compatibility glue so we can support IPv6 when it's compiled as a module */
>  struct pingv6_ops {
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index 6ae3345a3bdf..11d401958673 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -35,8 +35,8 @@ static int ip_ttl_max = 255;
>  static int tcp_syn_retries_min = 1;
>  static int tcp_syn_retries_max = MAX_TCP_SYNCNT;
>  static int tcp_syn_linear_timeouts_max = MAX_TCP_SYNCNT;
> -static int ip_ping_group_range_min[] = { 0, 0 };
> -static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
> +static unsigned long ip_ping_group_range_min[] = { 0, 0 };
> +static unsigned long ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
>  static u32 u32_max_div_HZ = UINT_MAX / HZ;
>  static int one_day_secs = 24 * 3600;
>  static u32 fib_multipath_hash_fields_all_mask __maybe_unused =
> @@ -165,8 +165,8 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
>                                  void *buffer, size_t *lenp, loff_t *ppos)
>  {
>         struct user_namespace *user_ns = current_user_ns();
> +       unsigned long urange[2];
>         int ret;
> -       gid_t urange[2];
>         kgid_t low, high;
>         struct ctl_table tmp = {
>                 .data = &urange,
> @@ -179,7 +179,7 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
>         inet_get_ping_group_range_table(table, &low, &high);
>         urange[0] = from_kgid_munged(user_ns, low);
>         urange[1] = from_kgid_munged(user_ns, high);
> -       ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> +       ret = proc_doulongvec_minmax(&tmp, write, buffer, lenp, ppos);
>
>         if (write && ret == 0) {
>                 low = make_kgid(user_ns, urange[0]);
> ---8<---


Will this work on 32bit build ?

Re: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Wed, 31 May 2023 23:09:02 +0200
> On Wed, May 31, 2023 at 9:19 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > From: ~akihirosuda <akihirosuda@git.sr.ht>
> > Date: Wed, 31 May 2023 19:42:49 +0900
> > > From: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
> > >
> > > With this commit, all the GIDs ("0 4294967294") can be written to the
> > > "net.ipv4.ping_group_range" sysctl.
> > >
> > > Note that 4294967295 (0xffffffff) is an invalid GID (see gid_valid() in
> > > include/linux/uidgid.h), and an attempt to register this number will cause
> > > -EINVAL.
> > >
> > > Prior to this commit, only up to GID 2147483647 could be covered.
> > > Documentation/networking/ip-sysctl.rst had "0 4294967295" as an example
> > > value, but this example was wrong and causing -EINVAL.
> > >
> > > In the implementation, proc_dointvec_minmax is no longer used because it
> > > does not support numbers from 2147483648 to 4294967294.
> >
> > Good catch.
> >
> > I think we can use proc_doulongvec_minmax() instead of open coding.
> >
> > With the diff below:
> >
> > ---8<---
> > # sysctl -a | grep ping
> > net.ipv4.ping_group_range = 0   2147483647
> > # sysctl -w net.ipv4.ping_group_range="0 4294967295"
> > sysctl: setting key "net.ipv4.ping_group_range": Invalid argument
> > # sysctl -w net.ipv4.ping_group_range="0 4294967294"
> > net.ipv4.ping_group_range = 0 4294967294
> > # sysctl -a | grep ping
> > net.ipv4.ping_group_range = 0   4294967294
> > ---8<---
> >
> > ---8<---
> > diff --git a/include/net/ping.h b/include/net/ping.h
> > index 9233ad3de0ad..9b401b9a9d35 100644
> > --- a/include/net/ping.h
> > +++ b/include/net/ping.h
> > @@ -20,7 +20,7 @@
> >   * gid_t is either uint or ushort.  We want to pass it to
> >   * proc_dointvec_minmax(), so it must not be larger than MAX_INT
> >   */
> > -#define GID_T_MAX (((gid_t)~0U) >> 1)
> > +#define GID_T_MAX ((gid_t)~0U)
> >
> >  /* Compatibility glue so we can support IPv6 when it's compiled as a module */
> >  struct pingv6_ops {
> > diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> > index 6ae3345a3bdf..11d401958673 100644
> > --- a/net/ipv4/sysctl_net_ipv4.c
> > +++ b/net/ipv4/sysctl_net_ipv4.c
> > @@ -35,8 +35,8 @@ static int ip_ttl_max = 255;
> >  static int tcp_syn_retries_min = 1;
> >  static int tcp_syn_retries_max = MAX_TCP_SYNCNT;
> >  static int tcp_syn_linear_timeouts_max = MAX_TCP_SYNCNT;
> > -static int ip_ping_group_range_min[] = { 0, 0 };
> > -static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
> > +static unsigned long ip_ping_group_range_min[] = { 0, 0 };
> > +static unsigned long ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
> >  static u32 u32_max_div_HZ = UINT_MAX / HZ;
> >  static int one_day_secs = 24 * 3600;
> >  static u32 fib_multipath_hash_fields_all_mask __maybe_unused =
> > @@ -165,8 +165,8 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
> >                                  void *buffer, size_t *lenp, loff_t *ppos)
> >  {
> >         struct user_namespace *user_ns = current_user_ns();
> > +       unsigned long urange[2];
> >         int ret;
> > -       gid_t urange[2];
> >         kgid_t low, high;
> >         struct ctl_table tmp = {
> >                 .data = &urange,
> > @@ -179,7 +179,7 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
> >         inet_get_ping_group_range_table(table, &low, &high);
> >         urange[0] = from_kgid_munged(user_ns, low);
> >         urange[1] = from_kgid_munged(user_ns, high);
> > -       ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> > +       ret = proc_doulongvec_minmax(&tmp, write, buffer, lenp, ppos);
> >
> >         if (write && ret == 0) {
> >                 low = make_kgid(user_ns, urange[0]);
> > ---8<---
> 
> 
> Will this work on 32bit build ?

It worked at least on my i686 build and qemu.

---8<---
# uname -a
Linux (none) 6.4.0-rc3-00648-g75455b906d82-dirty #76 SMP PREEMPT_DYNAMIC Wed May 31 21:30:31 UTC 2023 i686 GNU/Linux
# sysctl -a | grep ping
net.ipv4.ping_group_range = 1	0
# sysctl -w net.ipv4.ping_group_range="0 4294967295"
sysctl: setting key "net.ipv4.ping_group_range": Invalid argument
# sysctl -w net.ipv4.ping_group_range="0 4294967294"
net.ipv4.ping_group_range = 0 4294967294
# sysctl -a | grep ping
net.ipv4.ping_group_range = 0	4294967294
---8<---

Re: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[Akihiro Suda]

> I think we can use proc_doulongvec_minmax() instead of open coding.

Thanks, sent v2 with your suggestion
https://lore.kernel.org/lkml/168557950756.14226.6470993129419598644-0@git.sr.ht/

2023年6月1日(木) 8:00 Kuniyuki Iwashima <kuniyu@amazon.com>:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Wed, 31 May 2023 23:09:02 +0200
> > On Wed, May 31, 2023 at 9:19 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > >
> > > From: ~akihirosuda <akihirosuda@git.sr.ht>
> > > Date: Wed, 31 May 2023 19:42:49 +0900
> > > > From: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
> > > >
> > > > With this commit, all the GIDs ("0 4294967294") can be written to the
> > > > "net.ipv4.ping_group_range" sysctl.
> > > >
> > > > Note that 4294967295 (0xffffffff) is an invalid GID (see gid_valid() in
> > > > include/linux/uidgid.h), and an attempt to register this number will cause
> > > > -EINVAL.
> > > >
> > > > Prior to this commit, only up to GID 2147483647 could be covered.
> > > > Documentation/networking/ip-sysctl.rst had "0 4294967295" as an example
> > > > value, but this example was wrong and causing -EINVAL.
> > > >
> > > > In the implementation, proc_dointvec_minmax is no longer used because it
> > > > does not support numbers from 2147483648 to 4294967294.
> > >
> > > Good catch.
> > >
> > > I think we can use proc_doulongvec_minmax() instead of open coding.
> > >
> > > With the diff below:
> > >
> > > ---8<---
> > > # sysctl -a | grep ping
> > > net.ipv4.ping_group_range = 0   2147483647
> > > # sysctl -w net.ipv4.ping_group_range="0 4294967295"
> > > sysctl: setting key "net.ipv4.ping_group_range": Invalid argument
> > > # sysctl -w net.ipv4.ping_group_range="0 4294967294"
> > > net.ipv4.ping_group_range = 0 4294967294
> > > # sysctl -a | grep ping
> > > net.ipv4.ping_group_range = 0   4294967294
> > > ---8<---
> > >
> > > ---8<---
> > > diff --git a/include/net/ping.h b/include/net/ping.h
> > > index 9233ad3de0ad..9b401b9a9d35 100644
> > > --- a/include/net/ping.h
> > > +++ b/include/net/ping.h
> > > @@ -20,7 +20,7 @@
> > >   * gid_t is either uint or ushort.  We want to pass it to
> > >   * proc_dointvec_minmax(), so it must not be larger than MAX_INT
> > >   */
> > > -#define GID_T_MAX (((gid_t)~0U) >> 1)
> > > +#define GID_T_MAX ((gid_t)~0U)
> > >
> > >  /* Compatibility glue so we can support IPv6 when it's compiled as a module */
> > >  struct pingv6_ops {
> > > diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> > > index 6ae3345a3bdf..11d401958673 100644
> > > --- a/net/ipv4/sysctl_net_ipv4.c
> > > +++ b/net/ipv4/sysctl_net_ipv4.c
> > > @@ -35,8 +35,8 @@ static int ip_ttl_max = 255;
> > >  static int tcp_syn_retries_min = 1;
> > >  static int tcp_syn_retries_max = MAX_TCP_SYNCNT;
> > >  static int tcp_syn_linear_timeouts_max = MAX_TCP_SYNCNT;
> > > -static int ip_ping_group_range_min[] = { 0, 0 };
> > > -static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
> > > +static unsigned long ip_ping_group_range_min[] = { 0, 0 };
> > > +static unsigned long ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
> > >  static u32 u32_max_div_HZ = UINT_MAX / HZ;
> > >  static int one_day_secs = 24 * 3600;
> > >  static u32 fib_multipath_hash_fields_all_mask __maybe_unused =
> > > @@ -165,8 +165,8 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
> > >                                  void *buffer, size_t *lenp, loff_t *ppos)
> > >  {
> > >         struct user_namespace *user_ns = current_user_ns();
> > > +       unsigned long urange[2];
> > >         int ret;
> > > -       gid_t urange[2];
> > >         kgid_t low, high;
> > >         struct ctl_table tmp = {
> > >                 .data = &urange,
> > > @@ -179,7 +179,7 @@ static int ipv4_ping_group_range(struct ctl_table *table, int write,
> > >         inet_get_ping_group_range_table(table, &low, &high);
> > >         urange[0] = from_kgid_munged(user_ns, low);
> > >         urange[1] = from_kgid_munged(user_ns, high);
> > > -       ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> > > +       ret = proc_doulongvec_minmax(&tmp, write, buffer, lenp, ppos);
> > >
> > >         if (write && ret == 0) {
> > >                 low = make_kgid(user_ns, urange[0]);
> > > ---8<---
> >
> >
> > Will this work on 32bit build ?
>
> It worked at least on my i686 build and qemu.
>
> ---8<---
> # uname -a
> Linux (none) 6.4.0-rc3-00648-g75455b906d82-dirty #76 SMP PREEMPT_DYNAMIC Wed May 31 21:30:31 UTC 2023 i686 GNU/Linux
> # sysctl -a | grep ping
> net.ipv4.ping_group_range = 1   0
> # sysctl -w net.ipv4.ping_group_range="0 4294967295"
> sysctl: setting key "net.ipv4.ping_group_range": Invalid argument
> # sysctl -w net.ipv4.ping_group_range="0 4294967294"
> net.ipv4.ping_group_range = 0 4294967294
> # sysctl -a | grep ping
> net.ipv4.ping_group_range = 0   4294967294
> ---8<---

RE: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[David Laight]

From: Kuniyuki Iwashima
> Sent: 01 June 2023 00:00
....
> > > --- a/include/net/ping.h
> > > +++ b/include/net/ping.h
> > > @@ -20,7 +20,7 @@
> > >   * gid_t is either uint or ushort.  We want to pass it to
> > >   * proc_dointvec_minmax(), so it must not be larger than MAX_INT
> > >   */
> > > -#define GID_T_MAX (((gid_t)~0U) >> 1)
> > > +#define GID_T_MAX ((gid_t)~0U)

Doesn't that comment need updating?

I do wonder how much code breaks for gid > MAXINT.
How much testing does it actually get??

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294

[Akihiro Suda]

2023年6月2日(金) 19:51 David Laight <David.Laight@aculab.com>:
>
> From: Kuniyuki Iwashima
> > Sent: 01 June 2023 00:00
> ....
> > > > --- a/include/net/ping.h
> > > > +++ b/include/net/ping.h
> > > > @@ -20,7 +20,7 @@
> > > >   * gid_t is either uint or ushort.  We want to pass it to
> > > >   * proc_dointvec_minmax(), so it must not be larger than MAX_INT
> > > >   */
> > > > -#define GID_T_MAX (((gid_t)~0U) >> 1)
> > > > +#define GID_T_MAX ((gid_t)~0U)
>
> Doesn't that comment need updating?
>
> I do wonder how much code breaks for gid > MAXINT.
> How much testing does it actually get??

It is fixed in v3:
https://patchwork.kernel.org/project/netdevbpf/patch/20230601031305.55901-1-akihiro.suda.cz@hco.ntt.co.jp/

```
-/*
- * gid_t is either uint or ushort.  We want to pass it to
- * proc_dointvec_minmax(), so it must not be larger than MAX_INT
- */
-#define GID_T_MAX (((gid_t)~0U) >> 1)
+#define GID_T_MAX (((gid_t)~0U) - 1)
```

Tested with 4294967294, 4294967295 (EINVAL), and 4294967296 (EINVAL), on x86_64.




>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)