[PATCH ipsec] xfrm: avoid wrapping AEVENT timer thresholds

[PATCH ipsec] xfrm: avoid wrapping AEVENT timer thresholds

[Yingjie Wang]

net->xfrm.sysctl_aevent_etime is stored in 100ms units, but the
conversion in xfrm_state_construct() multiplies it by HZ while it is
still a u32. Large values can wrap before the division by
XFRM_AE_ETH_M, disabling the replay timer or turning a long threshold
into a short delay.

Do the conversion in u64 and clamp the resulting jiffies value to the
range representable by replay_maxage and safe for jiffies comparisons.
Use div_u64() so 32-bit builds do not require compiler runtime helpers
for the u64 division. Use widened arithmetic when reporting
XFRMA_ETIMER_THRESH as well, so large stored values do not wrap during
readback.

On x86-64 with HZ=1000, setting xfrm_aevent_etime to 536870912 made
XFRMA_ETIMER_THRESH read back as 0 on the base kernel and 42949672 with
this fix. A normal value of 8 read back as 8 on both kernels.

Fixes: d51d081d6504 ("[IPSEC]: Sync series - user")
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <flashhhhha@gmail.com>
---
 net/xfrm/xfrm_user.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index a5fbe38839c4..4a676a6aa53e 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -924,6 +924,14 @@ static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
 	}
 }
 
+static u32 xfrm_aevent_etime_to_jiffies(u32 etime)
+{
+	u64 maxage = min_t(u64, U32_MAX, MAX_JIFFY_OFFSET);
+	u64 j = div_u64((u64)etime * HZ, XFRM_AE_ETH_M);
+
+	return min_t(u64, j, maxage);
+}
+
 static struct xfrm_state *xfrm_state_construct(struct net *net,
 					       struct xfrm_usersa_info *p,
 					       struct nlattr **attrs,
@@ -1022,7 +1030,8 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 	x->km.seq = p->seq;
 	x->replay_maxdiff = net->xfrm.sysctl_aevent_rseqth;
 	/* sysctl_xfrm_aevent_etime is in 100ms units */
-	x->replay_maxage = (net->xfrm.sysctl_aevent_etime*HZ)/XFRM_AE_ETH_M;
+	x->replay_maxage =
+		xfrm_aevent_etime_to_jiffies(net->xfrm.sysctl_aevent_etime);
 
 	if ((err = xfrm_init_replay(x, extack)))
 		goto error;
@@ -2782,8 +2791,10 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
 			goto out_cancel;
 	}
 	if (id->flags & XFRM_AE_ETHR) {
+		u64 etime = div_u64((u64)x->replay_maxage * XFRM_AE_ETH_M, HZ);
+
 		err = nla_put_u32(skb, XFRMA_ETIMER_THRESH,
-				  x->replay_maxage * 10 / HZ);
+				  min_t(u64, etime, U32_MAX));
 		if (err)
 			goto out_cancel;
 	}
-- 
2.43.0

Re: [PATCH ipsec] xfrm: avoid wrapping AEVENT timer thresholds

[netdev-bot+sashiko]

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 patch clamps x->replay_maxage only on the sysctl path.

--

xfrm: avoid wrapping AEVENT timer thresholds

This patch converts net->xfrm.sysctl_aevent_etime to jiffies in u64 and
clamps the result to min(U32_MAX, MAX_JIFFY_OFFSET) before storing it in
x->replay_maxage. It also widens the arithmetic build_aevent() uses when
it reports XFRMA_ETIMER_THRESH, so large stored values do not wrap on
readback.

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index f802899d3d130..d7fa63aeea37b 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -924,6 +924,14 @@ static void xfrm_smark_init(struct nlattr **attrs, struct xfrm_mark *m)
>  	}
>  }
>  
> +static u32 xfrm_aevent_etime_to_jiffies(u32 etime)
> +{
> +	u64 maxage = min_t(u64, U32_MAX, MAX_JIFFY_OFFSET);
> +	u64 j = div_u64((u64)etime * HZ, XFRM_AE_ETH_M);
> +
> +	return min_t(u64, j, maxage);
> +}
> +
>  static struct xfrm_state *xfrm_state_construct(struct net *net,
>  					       struct xfrm_usersa_info *p,
>  					       struct nlattr **attrs,
> @@ -1022,7 +1030,8 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
>  	x->km.seq = p->seq;
>  	x->replay_maxdiff = net->xfrm.sysctl_aevent_rseqth;
>  	/* sysctl_xfrm_aevent_etime is in 100ms units */
> -	x->replay_maxage = (net->xfrm.sysctl_aevent_etime*HZ)/XFRM_AE_ETH_M;
> +	x->replay_maxage =
> +		xfrm_aevent_etime_to_jiffies(net->xfrm.sysctl_aevent_etime);

[Severity: Medium]
The commit message says the value is clamped to the range "safe for
jiffies comparisons". This clamp only applies to the sysctl default,
though.

A little further down, xfrm_state_construct() calls
xfrm_update_ae_params(x, attrs, 0). That call overwrites replay_maxage
with the raw attribute value whenever XFRMA_ETIMER_THRESH is present:

net/xfrm/xfrm_user.c:xfrm_update_ae_params() {
    ...
	if (et)
		x->replay_maxage = nla_get_u32(et);
    ...
}

xfrm_new_ae() reaches the same store on a live SA through
xfrm_update_ae_params(x, attrs, 1). The xfrma_policy entry for
XFRMA_ETIMER_THRESH is only NLA_U32, so it does no range check.

On 32-bit kernels, MAX_JIFFY_OFFSET is 0x3FFFFFFE. If a caller sends
XFRMA_ETIMER_THRESH >= 0x80000000, this call gives the timer a negative
delta:

    mod_timer(&x->rtimer, jiffies + x->replay_maxage);

The call appears in __xfrm_state_insert(), xfrm_replay_notify(),
xfrm_replay_notify_bmp() and xfrm_replay_notify_esn().
calc_wheel_index() treats a negative delta as already expired:

	} else if ((long) delta < 0) {
		idx = clk & LVL_MASK;
		*bucket_expiry = clk;

Would rtimer then fire on the next tick? If so, XFRM_MSG_NEWAE
notifications would no longer be rate-limited while the replay state
keeps changing.

This looks like the "turning a long threshold into a short delay" case
the commit message describes. It can still be reached through netlink
with CAP_NET_ADMIN in the netns, including from a user namespace.

Should xfrm_update_ae_params() apply the same clamp, or should the
XFRMA_ETIMER_THRESH policy carry a matching range?

>  
>  	if ((err = xfrm_init_replay(x, extack)))
>  		goto error;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114091239.780633-1-flashhhhha%40gmail.com