[PATCH net 1/2] netfilter: conntrack: avoid secs-to-jiffies overflow in SIP helper

[PATCH net 1/2] netfilter: conntrack: avoid secs-to-jiffies overflow in SIP helper

[luoqing]

From: Qing Luo <luoqing@kylinos.cn>

refresh_signalling_expectation() converts the expiry from a SIP
REGISTER 2xx response with "expires * HZ" in 32-bit arithmetic and
stores it in a u32.  sip_strtouint() only rejects values above
UINT_MAX, so the answering registrar controls the input.

Expectations are reaped with "(__s32)(exp->timeout - nfct_time_stamp)
<= 0".  At HZ=1000 an expiry of 2147484 seconds (24.9 days) crosses
2^31 and the expectation is born expired, while 4294968 seconds wraps
to 704 jiffies.  Registration refreshes break, or a signalling
expectation stays pinned for days.

Add nf_ct_secs_to_jiffies(), which multiplies in u64 and saturates at
INT_MAX so the signed expiry test stays positive, and use it for the
SIP expectation refresh.

Sweeping all 2^32-1 inputs at HZ=1000, the helper never wraps, never
yields a value that reads as expired, and matches the old expression
up to INT_MAX / HZ.

Fixes: 0f32a40fc91a ("[NETFILTER]: nf_conntrack_sip: create signalling expectations")
Cc: stable@vger.kernel.org
Signed-off-by: Qing Luo <luoqing@kylinos.cn>
---
 include/net/netfilter/nf_conntrack.h | 10 ++++++++++
 net/netfilter/nf_conntrack_sip.c     |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index c39425e54d87..b32716084ea8 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -210,6 +210,16 @@ bool nf_ct_get_tuplepr(const struct sk_buff *skb, unsigned int nhoff,
 void __nf_ct_refresh_acct(struct nf_conn *ct, enum ip_conntrack_info ctinfo,
 			  u32 extra_jiffies, unsigned int bytes);
 
+/* Convert a timeout in seconds to jiffies, saturating at INT_MAX instead of
+ * wrapping the "secs * HZ" product in u32.
+ */
+static inline u32 nf_ct_secs_to_jiffies(u32 secs)
+{
+	u64 delta = (u64)secs * HZ;
+
+	return delta > INT_MAX ? INT_MAX : (u32)delta;
+}
+
 /* Refresh conntrack for this many jiffies and do accounting */
 static inline void nf_ct_refresh_acct(struct nf_conn *ct,
 				      enum ip_conntrack_info ctinfo,
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 64bc440b1181..e2fd97f53386 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -891,7 +891,8 @@ static int refresh_signalling_expectation(struct nf_conn *ct,
 		    exp->tuple.dst.protonum != proto ||
 		    exp->tuple.dst.u.udp.port != port)
 			continue;
-		WRITE_ONCE(exp->timeout, nfct_time_stamp + (expires * HZ));
+		WRITE_ONCE(exp->timeout,
+			   nfct_time_stamp + nf_ct_secs_to_jiffies(expires));
 		WRITE_ONCE(exp->flags, exp->flags & ~NF_CT_EXPECT_INACTIVE);
 		found = 1;
 		break;
-- 
2.25.1

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

[luoqing]

From: Qing Luo <luoqing@kylinos.cn>

process_rcf() converts info->timeout with "* HZ" twice, once for the
conntrack refresh and once for the Q.931 expectation, both in 32-bit
arithmetic.  info->timeout is a u32 decoded from the ASN.1 timeToLive
field and only checked for being non-zero, so it is remote-controlled
by both the endpoint (RegistrationRequest) and the gatekeeper
(RegistrationConfirm).

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.

Reuse nf_ct_secs_to_jiffies() for both conversions and compute the
value once.

Sweeping all 2^32-1 inputs at HZ=1000, the helper never wraps, never
yields a value that reads as expired, and matches the old expression
up to INT_MAX / HZ; objdump shows the two 32-bit "imul $0x3e8" in
ras_help() replaced by a 64-bit multiply plus cmp/cmova saturation.

Fixes: f587de0e2feb ("[NETFILTER]: nf_conntrack/nf_nat: add H.323 helper port")
Cc: stable@vger.kernel.org
Signed-off-by: Qing Luo <luoqing@kylinos.cn>
---
 net/netfilter/nf_conntrack_h323_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 4cb1665bba02..5e900037fa6f 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);
+
 		pr_debug("nf_ct_ras: set RAS connection timeout to "
 			 "%u seconds\n", info->timeout);
-		nf_ct_refresh(ct, info->timeout * HZ);
+		nf_ct_refresh(ct, timeout);
 
 		/* Set expect timeout */
 		spin_lock_bh(&nf_conntrack_expect_lock);
@@ -1388,8 +1390,7 @@ static int process_rcf(struct sk_buff *skb, struct nf_conn *ct,
 				 "timeout to %u seconds for",
 				 info->timeout);
 			nf_ct_dump_tuple(&exp->tuple);
-			WRITE_ONCE(exp->timeout,
-				   nfct_time_stamp + (info->timeout * HZ));
+			WRITE_ONCE(exp->timeout, nfct_time_stamp + timeout);
 		}
 		spin_unlock_bh(&nf_conntrack_expect_lock);
 	}
-- 
2.25.1

Re: [PATCH net 1/2] netfilter: conntrack: avoid secs-to-jiffies overflow in SIP helper

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 1/2] netfilter: conntrack: avoid secs-to-jiffies overflow in SIP helper

[luoqing]

Hi,

Thank you for the reminder. I appreciate the automated check and the time
taken to review this series. I would like to provide the missing information
below. I apologize that it was not sufficiently clear in the original commit
messages.

- How the issue was discovered:
   The issue was noticed while I was studying this module with AI-assisted
   learning. The AI assistance helped me spot a suspicious code path, and I
   then verified it through manual code analysis / code inspection. It was
   not hit in production or during development, was not reported by syzbot,
   and was not found by an automated static analysis tool scan. The final
   conclusion is based on my own manual code analysis.

- Whether the issue was actually triggered:
   The issue has not been triggered in practice. It is currently a
   theoretical issue found by code inspection, so there are no stack traces
   or error messages to report. I understand this is different from a
   reproduced failure.

I am not reposting the series just to add this information. Please take
this reply into account during your review. If another revision is needed
for other reasons, I will include this information in the commit messages
then.

Thanks again for pointing this out.

Best regards,

luoqing

-------- 原始消息 --------
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>   - How the issue was discovered, e.g. hit in production, hit during
>     development, syzbot report, manual code inspection, LLM or static
>     analysis tool scan.
>
>   - Whether the issue was actually triggered, or is only theoretical
>     (e.g. found by code inspection). If it was triggered please include
>     the symptoms, like the stack trace or error messages.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Re: [PATCH net 1/2] netfilter: conntrack: avoid secs-to-jiffies overflow in SIP helper

[netdev-bot+sashiko]

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] The Fixes: tag gives the wrong origin for the failure the commit
  message mainly describes, which matters for backporting.
- [Low] This is a bug fix under net/, which MAINTAINERS 'NETWORKING
  [GENERAL]' covers with 'P: Documentation/process/maintainer-…

--

netfilter: conntrack: avoid secs-to-jiffies overflow in SIP helper

This adds nf_ct_secs_to_jiffies(), which multiplies in u64 and saturates
at INT_MAX. refresh_signalling_expectation() now uses it, so a large
expiry in a SIP REGISTER 2xx response can no longer wrap the u32
exp->timeout or make the expectation read as already expired.

> Expectations are reaped with "(__s32)(exp->timeout - nfct_time_stamp)
> <= 0".  At HZ=1000 an expiry of 2147484 seconds (24.9 days) crosses
> 2^31 and the expectation is born expired, while 4294968 seconds wraps
> to 704 jiffies.  Registration refreshes break, or a signalling
> expectation stays pinned for days.

[ ... ]

> Fixes: 0f32a40fc91a ("[NETFILTER]: nf_conntrack_sip: create signalling expectations")
> Cc: stable@vger.kernel.org

[Severity: Low]
Should this also have a Fixes: tag for b8b09dc2bf35 ("netfilter:
nf_conntrack_expect: use conntrack GC to reap expectations")?

The commit message quotes a signed expiry test. That test is in
nf_ct_exp_is_expired():

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

b8b09dc2bf35 added that test. It also made exp->timeout a u32 and added
the line this patch changes in refresh_signalling_expectation():

    WRITE_ONCE(exp->timeout, nfct_time_stamp + (expires * HZ));

0f32a40fc91a used a timer instead:

    exp->timeout.expires = jiffies + expires * HZ;
    add_timer(&exp->timeout);

Later trees changed this to mod_timer_pending(), but still added
jiffies + expires * HZ.

Trees from before b8b09dc2bf35 only have the 2^32 wrap (4294968 seconds
turning into 704 jiffies). On 64-bit, a product between 2^31 and 2^32 is a
positive unsigned long delta, and the timer wheel clamps it. So the
24.9 day "born expired" case does not seem to exist in those trees.

Those older trees also do not have the WRITE_ONCE(exp->timeout, ...)
context, so the stable backport will not apply cleanly there.

Could the tags or the message say which failure mode applies to which
trees?

The h323 patch in this series ("netfilter: h323: avoid secs-to-jiffies
overflow in RAS timeout") has the same pattern. It cites only
f587de0e2feb, but it relies on the same signed expiry test.

> Sweeping all 2^32-1 inputs at HZ=1000, the helper never wraps, never
> yields a value that reads as expired, and matches the old expression
> up to INT_MAX / HZ.

[Severity: Low]
Could the commit message say how this issue was found, and how the fix
was tested beyond the arithmetic sweep of nf_ct_secs_to_jiffies()?

net/netfilter/ is covered by the NETWORKING [GENERAL] entry, which points
to Documentation/process/maintainer-netdev.rst. Its bug fixes section
says:

    All bug fixes must state how the issue was discovered and validated,
    and how the fix was tested.

It also asks whether the fix was tested against the reproducer and the
kernel selftests.

The message says "Registration refreshes break, or a signalling
expectation stays pinned for days". It does not say whether this was
seen with real REGISTER 2xx traffic through the SIP helper, or found by
reading the code. There is also no Reported-by: tag.

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

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