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

[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 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