Re: [PATCH net 2/2] netfilter: h323: avoid secs-to-jiffies overflow in RAS timeout

[netdev-bot+sashiko]

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

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

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 names a symptom that the overflow cannot
  cause and that the patch does not fix.
- [Low] process_rcf() calls nf_ct_secs_to_jiffies().

--

netfilter: h323: avoid secs-to-jiffies overflow in RAS timeout

This patch replaces the two 32-bit "info->timeout * HZ" conversions in
process_rcf() with one call to nf_ct_secs_to_jiffies(). That helper
multiplies in 64 bits and saturates at INT_MAX. The result is used for
both the RAS conntrack refresh and the Q.931 expectation timeout.

> With the signed expiry test "(__s32)(ct->timeout - nfct_time_stamp)
> <= 0", a timeout of 2147484 seconds (24.9 days) at HZ=1000 is born
> expired, and 4294968 seconds wraps to 704 jiffies.  The RAS connection
> therefore drops mid-call, or the Q.931 expectation stays pinned for
> days after the peer is gone.

[Severity: Medium]
Is the second half of this accurate? Can the 32-bit wrap of
info->timeout * HZ ever make a lifetime longer than the one requested?

For X > INT_MAX / HZ, the old value X * HZ mod 2^32 ends up in one of two
places. If it is >= 2^31, it reads as already expired under
nf_ct_exp_is_expired():

    return (__s32)(READ_ONCE(exp->timeout) - nfct_time_stamp) <= 0;

If it is below 2^31, it is smaller than X * HZ. Either way the lifetime
is shorter than what the peer asked for, never longer.

After this patch, the timeToLive stored by process_rrq() or process_rcf()
still has no upper bound. The only limit is the INT_MAX jiffies
saturation in nf_ct_secs_to_jiffies(). That is about 24.8 days at HZ=1000
and about 248 days at HZ=100.

A peer could already reach that same maximum before this patch by
sending 2147483 seconds. So the longest lifetime a remote peer can force
on the RAS conntrack and the Q.931 expectation is the same before and
after the patch.

For oversized TTLs the change actually goes the other way. Values that
used to wrap to a short lifetime or come out already expired now get the
maximum lifetime. For example, 0xFFFFFFFF used to come out as s32 -1000
at HZ=1000.

Could the commit message drop the "pinned for days" symptom? It could
instead say directly that oversized TTLs now saturate to INT_MAX jiffies.

> diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
> index 4cb1665bba021..5e900037fa6f1 100644
> --- a/net/netfilter/nf_conntrack_h323_main.c
> +++ b/net/netfilter/nf_conntrack_h323_main.c
> @@ -1375,9 +1375,11 @@ static int process_rcf(struct sk_buff *skb, struct nf_conn *ct,
>  	}
>  
>  	if (info->timeout > 0) {
> +		u32 timeout = nf_ct_secs_to_jiffies(info->timeout);

[Severity: Low]
nf_ct_secs_to_jiffies() only exists because the previous patch in this
series, "netfilter: conntrack: avoid secs-to-jiffies overflow in SIP
helper", adds it to include/net/netfilter/nf_conntrack.h. That patch
carries a different Fixes: tag (0f32a40fc91a, where this one has
f587de0e2feb).

Will this still build if it is backported to stable without the SIP
patch? That could happen if the SIP patch does not apply cleanly to an
older tree.

The SIP patch is also tagged for stable and comes first in the series,
so stable tooling will usually pick up both. Still, would it help to
note the dependency on the Cc: stable line?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104085925.773471-1-l1138897701%40163.com