Re: [PATCH nf-next v4 1/4] netfilter: conntrack: add shared uint and port parsers for helpers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] nf_ct_helper_parse_uint() rejects any run of 12 or more digits,
  even when the value fits in UINT_MAX, and neither comment mentions…

--

netfilter: conntrack: add shared uint and port parsers for helpers

This commit adds nf_ct_helper_parse_uint() and nf_ct_helper_parse_port()
to the conntrack helper core. Both parse decimal numbers from a buffer of
a given length. The uint parser body is taken from sip_strtouint(), and
both functions are exported for the IRC, Amanda and SIP helpers.

> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index c30ae3f203bee..99b90a54d8a9f 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c

[ ... ]

> @@ -569,6 +570,73 @@ void nf_nat_helper_unregister(struct nf_conntrack_nat_helper *nat)
>  }
>  EXPORT_SYMBOL_GPL(nf_nat_helper_unregister);
>  
> +/* Parse a decimal unsigned integer from a length-delimited buffer that is not
> + * necessarily NUL-terminated.  At most @len bytes are examined.  On success
> + * the parsed value is returned and, if @endp is non-NULL, *@endp points just
> + * past the last digit consumed.  If no digit is found or the value would
> + * exceed UINT_MAX, 0 is returned and *@endp is set to @cp.
> + */
> +unsigned int nf_ct_helper_parse_uint(const char *cp, unsigned int len,
> +				     char **endp)
> +{
> +	const unsigned int max = sizeof("4294967295");
> +	unsigned int olen = len;
> +	const char *s = cp;
> +	u64 result = 0;
> +
> +	if (len > max)
> +		len = max;
> +
> +	while (olen > 0 && isdigit(*s)) {
> +		unsigned int value;
> +
> +		if (len == 0)
> +			goto err;

[Severity: Low]
Should a run of 12 or more digits be rejected here when the value still
fits in UINT_MAX?

max is sizeof("4294967295"), which is 11 because sizeof counts the NUL.
For longer inputs, len is clamped to 11 while olen keeps the real length.

The loop condition only checks olen and isdigit(). So a 12th digit enters
the loop and finds len == 0. It then takes the err path, which returns 0
and sets *endp to cp.

With leading zeros, result stays small, so this failure comes only from
the digit count. The count limit does not seem to add overflow
protection. result is a u64 and is compared against UINT_MAX after every
digit.

The comment above lists only "no digit is found" and "exceed UINT_MAX" as
failure cases. The nf_ct_helper_parse_port() comment lists only "no digit
is found" and "out of range".

nf_ct_helper_parse_port() treats e == cp as a failure:

	val = nf_ct_helper_parse_uint(cp, len, &e);
	if (e == cp || val == 0 || val > 65535)
		return -1;

So an input like "000000000080" returns -1 there.

This logic came from sip_strtouint(), but it is now a general exported
API. Later in the series, "netfilter: nf_conntrack_irc: use
nf_ct_helper_parse_port()" replaces simple_strtoul() in parse_dcc().
simple_strtoul() accepted any number of leading zeros.

Would a DCC command such as "DCC SEND f <ip> 0000000000006667" then fail
to parse, so that no expectation is created for it?

Amanda does not look affected, because pbuf limits its input to 5
characters.

Could the digit-count limit be dropped, or else documented in both
comments?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923091608.2617604-1-rc%40rexion.ai