Re: [PATCH v3 net 2/4] neighbour: Enforce min/max to NDTPA_INTERVAL_PROBE_TIME_MS.

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] The sysctl half of this patch is inert in this tree:
  `neigh_proc_dointvec_ms_jiffies_positive()`…

--

neighbour: Enforce min/max to NDTPA_INTERVAL_PROBE_TIME_MS.

The patch replaces the ineffective .min-only policy entry for
NDTPA_INTERVAL_PROBE_TIME_MS with NLA_POLICY_FULL_RANGE(), bounding it to
1..86400000 ms, and adds the same max to the sysctl handler plus the
netlink spec and ip-sysctl documentation.

One question about the sysctl half of the change below.

> diff --git a/Documentation/netlink/specs/rt-neigh.yaml b/Documentation/netlink/specs/rt-neigh.yaml
> index 0f46ef3135905..c8e55c98d5649 100644
> --- a/Documentation/netlink/specs/rt-neigh.yaml
> +++ b/Documentation/netlink/specs/rt-neigh.yaml
> @@ -341,6 +341,9 @@ attribute-sets:
>        -
>          name: interval-probe-time-ms
>          type: u64
> +        checks:
> +          min: 1
> +          max: 86400000
>  
>  operations:
>    enum-model: directional
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 208f46967ee59..b05829e44d8fc 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -248,7 +248,7 @@ neigh/default/unres_qlen - INTEGER
>  
>  neigh/default/interval_probe_time_ms - INTEGER
>  	The probe interval for neighbor entries with NTF_MANAGED flag,
> -	the min value is 1.
> +	the min value is 1, and the max value is 86400000 (1 day).
>  
>  	Default: 5000
>  
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 49dd7df149ef3..0db78a0dfb516 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2359,6 +2359,13 @@ static const struct nla_policy nl_neightbl_policy[NDTA_MAX+1] = {
>  	[NDTA_PARMS]		= { .type = NLA_NESTED },
>  };
>  
> +#define NTBL_PARM_MS_MAX	(24 * 60 * 60 * MSEC_PER_SEC)
> +
> +static const struct netlink_range_validation nl_ntbl_parm_ms_range = {
> +	.min = 1,
> +	.max = NTBL_PARM_MS_MAX,
> +};
> +

[ ... ]

> @@ -3672,12 +3680,13 @@ static int neigh_proc_dointvec_ms_jiffies_positive(const struct ctl_table *ctl,
>  						   void *buffer, size_t *lenp, loff_t *ppos)
>  {
>  	struct ctl_table tmp = *ctl;
> -	int ret;
> +	int ret, min, max;
>  
> -	int min = msecs_to_jiffies(1);
> +	min = msecs_to_jiffies(1);
> +	max = msecs_to_jiffies(NTBL_PARM_MS_MAX);
>  
>  	tmp.extra1 = &min;
> -	tmp.extra2 = NULL;
> +	tmp.extra2 = &max;
>  
>  	ret = proc_dointvec_ms_jiffies_minmax(&tmp, write, buffer, lenp, ppos);

[Severity: High]
Does proc_dointvec_ms_jiffies_minmax() actually look at extra1/extra2 in
this tree?  The call chain from neigh_proc_dointvec_ms_jiffies_positive()
seems to drop the range check:

kernel/time/jiffies.c:

  static int do_proc_int_conv_ms_jiffies_minmax(bool *negp, ulong *u_ptr,
                                                int *k_ptr, int dir,
                                                const struct ctl_table *tbl)
  {
        return proc_int_conv(negp, u_ptr, k_ptr, dir, tbl, false,
                             sysctl_u2k_int_conv_ms, sysctl_k2u_int_conv_ms);
  }

kernel/sysctl.c:proc_int_conv() only consults the bounds when
k_ptr_range_check is true:

        if (k_ptr_range_check) {
                ...
                if ((tbl->extra1 && *(int *)tbl->extra1 > tmp_k) ||
                    (tbl->extra2 && *(int *)tbl->extra2 < tmp_k))
                        return -EINVAL;
                WRITE_ONCE(*k_ptr, tmp_k);
        } else
                return user_to_kern(negp, u_ptr, k_ptr);

Since the converter passes false, aren't both tmp.extra1 = &min and the new
tmp.extra2 = &max dead stores, so a write of 0, of a value well above
86400000, or of a negative value to
/proc/sys/net/ipv{4,6}/neigh/*/interval_probe_time_ms still succeeds?

If so, the failure mode described in the commit message stays reachable via
the sysctl entry point, where neigh_managed_work() requeues itself with the
unvalidated value:

  queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
                     NEIGH_VAR(&tbl->parms, INTERVAL_PROBE_TIME_MS));

with 0 meaning no delay and a negative int sign-extending into the unsigned
long delay argument.

That would also make this sentence in the commit message inaccurate:

    The same max check is applied to sysctl as well.

and likewise the new documentation text:

	the min value is 1, and the max value is 86400000 (1 day).

The netlink half looks fine, so the sysctl path and the range now declared
in Documentation/netlink/specs/rt-neigh.yaml would disagree: an out-of-range
sysctl write can still be reported back by RTM_GETNEIGHTBL as a value
outside the declared 1..86400000 range.

The cover note below the --- line points at a separate, not-yet-merged
sysctl-core fix
(https://lore.kernel.org/linux-fsdevel/20260905233819.1064529-2-kuniyu@google.com/)
as a prerequisite.  Since that note does not end up in git history, would it
make sense to order this patch behind that fix, or to mention the dependency
in the commit message itself?  Note this commit carries a Fixes: tag, so a
stable backport would carry the documented bound without the enforcement.

I checked the end of this series and do_proc_int_conv_ms_jiffies_minmax()
still passes false, so nothing later in the series appears to restore the
check.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909233143.2401847-1-kuniyu%40google.com