Re: [PATCH net] ipv6: bound temporary address regeneration advance

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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 only regen_min_advance,
  regen_max_retry and dad_transmits as inputs to the overflowing int…

Pre-existing issues:
- [Low] In addrconf_verify_rtnl(), the deadline expressions `ifp->tstamp
  + ifp->prefered_lft * HZ` (lines 4727, 4744, 4783) multiply a __u32 by…

--

ipv6: bound temporary address regeneration advance

The commit makes the regen_min_advance, regen_max_retry and dad_transmits
sysctls reject negative values. It also reworks ipv6_get_regen_advance()
to use checked u64 arithmetic capped at U32_MAX. In ipv6_create_tempaddr()
the minimum preferred lifetime is now computed in u64 before narrowing,
and max_desync_factor is computed without an unsigned wrap.

ipv6_get_regen_advance() combines regen_min_advance, regen_max_retry
and dad_transmits. All three are signed sysctls, and the latter two
are multiplied as int. Negative values or overflow of large positive
inputs can make the unsigned long result much larger than intended.

[Severity: Medium]
Is this description complete? The old expression also multiplied by
NEIGH_VAR(idev->nd_parms, RETRANS_TIME). That value can come from an
on-link Router Advertisement, not only from a sysctl.

ndisc_router_discovery() in net/ipv6/ndisc.c stores the RA Retrans Timer
in the int RETRANS_TIME slot:

	if (in6_dev->nd_parms) {
		unsigned long rtime = ntohl(ra_msg->retrans_timer);

		if (rtime && rtime/1000 < MAX_SCHEDULE_TIMEOUT/HZ) {
			rtime = (rtime*HZ)/1000;
			if (rtime < HZ/100)
				rtime = HZ/100;
			NEIGH_VAR_SET(in6_dev->nd_parms, RETRANS_TIME, rtime);

On 64-bit, the rtime/1000 < MAX_SCHEDULE_TIMEOUT/HZ check always passes
for a u32. The only gate is ipv6_accept_ra(), which is on by default for
hosts.

Take the default sysctls (2/5/1), HZ=1000 and an RA Retrans Timer of
858992859 ms. The old int expression becomes:

    2 + (5 * 1 * 858992859) / 1000
  = 2 + (-3001) / 1000
  = 2 + (-3)
  = -1

So the old ipv6_get_regen_advance() returned ULONG_MAX. That is the same
state as the dad_transmits=-1 reproducer.

With age 0, the old ipv6_create_tempaddr() then set cfg.preferred_lft to
(u32)(ULONG_MAX + 1) = 0. Temporary addresses were therefore created
already deprecated.

Sometimes the new address does not stay tentative (accept_dad=0, or
IFF_NOARP devices). Then addrconf_verify_rtnl() sees
age + ULONG_MAX >= 0 and restarts its scan. This gives the same
address-creation loop under RTNL, and any RA that refreshes the public
address tstamp can start it again.

The new check_mul_overflow() in ipv6_get_regen_advance() does close this
path, because retrans_time now goes through the checked u64 multiply.

Could the commit message mention the RA-supplied Retrans Timer as an
unprivileged on-link trigger, next to the privileged sysctl reproducers?
That would matter for stable and security triage.

[ ... ]

[Severity: Low]
This isn't a bug introduced by this patch, but can these deadline
calculations in addrconf_verify_rtnl() wrap by themselves?

ifp->prefered_lft is __u32 and HZ is an int constant, so
ifp->prefered_lft * HZ is computed in 32-bit unsigned arithmetic. At
HZ=1000 it wraps for preferred lifetimes above about 4294967 seconds.

The same pattern shows up further down in this function:

				} else if (time_before(ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ, next))
					next = ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ;
    ...
				/* ifp->prefered_lft <= ifp->valid_lft */
				if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))
					next = ifp->tstamp + ifp->prefered_lft * HZ;

If next ends up in the past, the clamp at the end of the function
applies:

	if (time_before(next_sched, jiffies + ADDRCONF_TIMER_FUZZ_MAX))
		next_sched = jiffies + ADDRCONF_TIMER_FUZZ_MAX;

Would addr_chk_work then be rescheduled every ADDRCONF_TIMER_FUZZ_MAX
until the real deadline? Each run would do a needless full scan of the
address table under RTNL.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_F6E8C7CAC6D390C8E912EEE4FD9FE4333505%40qq.com