Re: [PATCH nf-next v4 4/4] netfilter: nf_conntrack_sip: use shared helper parsers

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says "No functional change intended", but
  the patch changes how sip_parse_port() in…

--

netfilter: nf_conntrack_sip: use shared helper parsers

This patch removes the SIP helper's private sip_strtouint() and switches
its callers to nf_ct_helper_parse_uint(). The digit loop in
sip_parse_port() is replaced with nf_ct_helper_parse_port(), while the
SIP-specific handling of ':', the SIP_PORT default and the 1024 minimum
stay in place. The commit message says no functional change is intended.

> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
> index 64bc440b1181f..0e12426227a80 100644
> --- a/net/netfilter/nf_conntrack_sip.c
> +++ b/net/netfilter/nf_conntrack_sip.c

[ ... ]

> @@ -199,29 +199,22 @@ static bool sip_parse_port(const char *dptr, const char **endp,
>  
>  	dptr++; /* skip ':' */
>  
> -	while (dptr < limit && isdigit(*dptr)) {
> -		p = p * 10 + (*dptr - '0');
> -		dptr++;
> -		len++;
> -		if (len > 5) /* max "65535" */
> -			return false;
> -	}
> -
> -	if (len == 0)
> +	if (nf_ct_helper_parse_port(dptr, limit - dptr, &p, &end))
>  		return false;

[Severity: Medium]
Does this change which ports sip_parse_port() accepts?

The old loop counted digits and returned false once there were more than
five, whatever the value. That rejected zero-padded ports such as
"sip:1.2.3.4:0005060".

The shared helper only checks the value:

net/netfilter/nf_conntrack_helper.c:nf_ct_helper_parse_port() {
    ...
	val = nf_ct_helper_parse_uint(cp, len, &e);
	if (e == cp || val == 0 || val > 65535)
		return -1;
    ...
}

nf_ct_helper_parse_uint() caps the digit count only at
sizeof("4294967295"), which is 11. As a result, ports of 6 to 11 digits
with leading zeros, such as ":0005060" or ":00000065535", now parse.

That appears to change the outcome for the same packet in several
callers:

  - ct_sip_parse_header_uri() used to return -1 for these strings, and
    process_register_request() and process_register_response() then
    returned NF_DROP with "cannot parse contact". It now returns 1, so a
    REGISTER request creates a signalling expectation:

    sip_help_udp/tcp()->process_sip_msg()->process_sip_request()->
      process_register_request()->ct_sip_parse_header_uri()->
        sip_parse_port()

  - ct_sip_parse_request() used to return -1 and now succeeds.

  - epaddr_len() and skp_epaddr_len(), the match_len callbacks used by
    ct_sip_get_header() and ct_sip_walk_headers(), used to return 0,
    which made the lookup return -1. Now the header matches with a
    longer matchlen, and nf_nat_sip mangles that range.

  - process_sip_request() may now set ct_sip_info->forced_dport from a
    zero-padded Via header that it used to ignore.

The sip_strtouint() to nf_ct_helper_parse_uint() swap looks identical.
The sip_parse_port() conversion is not, and the commit message quoted
below does not mention it:

  "No functional change intended."

Should the old limit be kept, for example by rejecting end - dptr > 5?
Otherwise, could the commit message document that leading zeros are now
accepted?

>  
>  	/* reached limit while parsing port */
> -	if (dptr >= limit)
> +	if (end >= limit)
>  		return false;

[ ... ]

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