Re: [PATCH linux] net/ipv4: ping_group_range: allow GID from 2147483648 to 4294967294
From: Kuniyuki Iwashima
Date: Wed May 31 2023 - 15:19:35 EST
From: ~akihirosuda <akihirosuda@xxxxxxxxx>
Date: Wed, 31 May 2023 19:42:49 +0900
> From: Akihiro Suda <akihiro.suda.cz@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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