Re: [PATCH nf-next v4 2/4] netfilter: nf_conntrack_irc: use nf_ct_helper_parse_port()

[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] After this patch, the `|| dcc_port == 0` test in help()
  (net/netfilter/nf_conntrack_irc.c) can never be true.
- [Low] Switching parse_dcc() to nf_ct_helper_parse_port() adds a digit-
  count limit to DCC port parsing that the kernel-doc does not describe.

--

netfilter: nf_conntrack_irc: use nf_ct_helper_parse_port()

This commit replaces simple_strtoul() in parse_dcc() with
nf_ct_helper_parse_port(). DCC ports above 65535 are no longer silently
truncated into the u16 port. The new helper parses a length-delimited
buffer and rejects port 0 and values above 65535.

> diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
> index 92360963757a9..268ce7c1f9b23 100644
> --- a/net/netfilter/nf_conntrack_irc.c
> +++ b/net/netfilter/nf_conntrack_irc.c
> @@ -88,7 +88,9 @@ static int parse_dcc(char *data, const char *data_end, __be32 *ip,
>  		data++;
>  	}
>  
> -	*port = simple_strtoul(data, &data, 10);
> +	if (nf_ct_helper_parse_port(data, data_end - data, port, &data))
> +		return -1;

[Severity: Low]

nf_ct_helper_parse_port() now returns -1 when val == 0. Can the
dcc_port == 0 test in help() still be true after that?

net/netfilter/nf_conntrack_irc.c:help() {
    ...
			if ((tuple->src.u3.ip != dcc_ip &&
			     ct->tuplehash[!dir].tuple.dst.u3.ip != dcc_ip) ||
			    dcc_port == 0) {
				net_warn_ratelimited("Forged DCC command from %pI4: %pI4:%u\n",
    ...
}

Before this patch, each of these inputs left dcc_port at 0: a port of
"0", a non-numeric port, or a value that truncated to 0 such as 65536.
In those cases parse_dcc() succeeded, and help() printed the ratelimited
"Forged DCC command" warning before goto out.

Now those commands fail in parse_dcc(), and help() takes this path
instead:

			if (parse_dcc(data, data_limit, &dcc_ip,
				       &dcc_port, &addr_beg_p, &addr_end_p)) {
				pr_debug("unable to parse dcc command\n");
				goto out;
			}

The result is the same either way: no expectation is created and the
packet gets NF_ACCEPT. The only difference is that the warning is no
longer printed. The dcc_port == 0 clause is still in help() at the end
of the series.

Should that clause be removed, with the logging change mentioned in the
commit message? Reverse/passive DCC also uses port 0, so it may be fine
to drop the "Forged" warning.

[Severity: Low]

Does this also reject in-range ports written with leading zeros?
nf_ct_helper_parse_uint() clamps len to 11 and counts every digit it
consumes, including leading zeros:

net/netfilter/nf_conntrack_helper.c:nf_ct_helper_parse_uint() {
	const unsigned int max = sizeof("4294967295");
    ...
	if (len > max)
		len = max;

	while (olen > 0 && isdigit(*s)) {
		unsigned int value;

		if (len == 0)
			goto err;
    ...
}

A DCC port such as "000000000001" reaches goto err on the 12th digit,
so parse_dcc() returns -1. simple_strtoul() used to accept that string
as port 1. help() now does goto out without creating an expectation or
calling the NAT hook.

The kernel-doc for nf_ct_helper_parse_uint() lists only two failure
cases: "no digit is found or the value would exceed UINT_MAX". The
comment for nf_ct_helper_parse_port() says "no digit is found or the
value is out of range". Neither one mentions the digit limit.

This logic comes from sip_strtouint(), so SIP already behaves this way,
but IRC did not. Real DCC clients probably don't zero-pad ports like
this. Could the helper documentation, or this commit message, mention
the limit?

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