[PATCH net] ipv6: bound temporary address regeneration advance

[PATCH net] ipv6: bound temporary address regeneration advance

[Yingjie Wang]

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.

When ipv6_create_tempaddr() clamps the preferred lifetime, it adds age
and one to this advance and stores it in a u32. The sum can wrap or be
truncated to a short preferred lifetime. addrconf_verify_rtnl() then
finds the new address due for regeneration and restarts its scan after
creating another one. The temporary-address path has had no
max_addresses check since commit 4b08a8f1bd8c ("ipv6: remove
max_addresses check from ipv6_create_tempaddr"). In a baseline namespace
test, setting dad_transmits=-1 and adding a temporary-address template
with mngtmpaddr produced 2859 loopback addresses in two seconds.

Non-negative inputs can also overflow the int product, so rejecting
negative writes alone is not enough.

Reject negative values for all three sysctls with -EINVAL. Writes that
used to succeed now fail validation; negative values have no useful meaning
(negative regen_max_retry already disables temporary addresses on the
first attempt). Keep zero valid. Use checked u64 arithmetic and
div_u64() to calculate the advance, and cap it at U32_MAX. Compare the
required preferred lifetime in u64 before narrowing it to u32. Calculate
the desync margin only when the configured lifetime exceeds the advance,
avoiding an unsigned subtraction wrap on 32-bit builds.

The issue was found and this fix was prepared with assistance from
OpenAI Codex.

Tested on x86-64 KASAN:
- negative writes to the three sysctls were rejected without changing
  their values, and zero remained valid;
- normal temporary-address setup produced three IPv6 addresses;
- with HZ=1000 and a one-second retransmission time,
  regen_min_advance=0, regen_max_retry=2147483647 and dad_transmits=1
  caused the baseline kernel to reach 2847 loopback addresses after
  two seconds; the fixed kernel left two.

Kernel selftests were not run.

An i386 defconfig with CONFIG_IPV6=y and HZ=1000 built addrconf.o with
W=1; nm -u showed no __udivdi3 reference.

Fixes: f4bcbf360ac8 ("net: ipv6/addrconf: clamp preferred_lft to the minimum required")
Signed-off-by: Yingjie Wang <1075151112@qq.com>
---
 Documentation/networking/ip-sysctl.rst |  3 ++
 net/ipv6/addrconf.c                    | 50 ++++++++++++++++++--------
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 4b19c8d3efda7..e78a59ebc9df2 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -2872,6 +2872,7 @@ dad_transmits - INTEGER
 	The amount of Duplicate Address Detection probes to send.
 
 	Default: 1
+	Minimum: 0
 
 forwarding - INTEGER
 	Configure interface-specific Host/Router behaviour.
@@ -3023,12 +3024,14 @@ regen_min_advance - INTEGER
 	does not conform to RFC 8981.
 
 	Default: 2
+	Minimum: 0
 
 regen_max_retry - INTEGER
 	Number of attempts before give up attempting to generate
 	valid temporary addresses.
 
 	Default: 5
+	Minimum: 0
 
 max_addresses - INTEGER
 	Maximum number of autoconfigured addresses per interface.  Setting
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index c8c0dd04982d9..39ecb2c25055b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -60,7 +60,9 @@
 #endif
 #include <linux/capability.h>
 #include <linux/delay.h>
+#include <linux/math64.h>
 #include <linux/notifier.h>
+#include <linux/overflow.h>
 #include <linux/string.h>
 #include <linux/hash.h>
 
@@ -1376,10 +1378,19 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 
 static unsigned long ipv6_get_regen_advance(const struct inet6_dev *idev)
 {
-	return READ_ONCE(idev->cnf.regen_min_advance) +
-		READ_ONCE(idev->cnf.regen_max_retry) *
-		READ_ONCE(idev->cnf.dad_transmits) *
-		max(NEIGH_VAR(idev->nd_parms, RETRANS_TIME), HZ/100) / HZ;
+	unsigned int retrans_time = max(NEIGH_VAR(idev->nd_parms,
+						 RETRANS_TIME), HZ / 100);
+	u32 min_advance = READ_ONCE(idev->cnf.regen_min_advance);
+	u32 retries = READ_ONCE(idev->cnf.regen_max_retry);
+	u32 dad = READ_ONCE(idev->cnf.dad_transmits);
+	u64 duration;
+
+	/* Preferred lifetimes are u32, so a larger advance cannot be used. */
+	if (check_mul_overflow((u64)retries * dad, (u64)retrans_time,
+			       &duration))
+		return U32_MAX;
+
+	return min_t(u64, (u64)min_advance + div_u64(duration, HZ), U32_MAX);
 }
 
 static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
@@ -1394,6 +1405,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
 	struct ifa6_config cfg;
 	long max_desync_factor;
 	struct in6_addr addr;
+	u64 min_preferred_lft;
 	int ret = 0;
 
 	write_lock_bh(&idev->lock);
@@ -1430,9 +1442,13 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
 	 * idev->desync_factor if it's larger
 	 */
 	cnf_temp_preferred_lft = READ_ONCE(idev->cnf.temp_prefered_lft);
-	max_desync_factor = min_t(long,
-				  READ_ONCE(idev->cnf.max_desync_factor),
-				  cnf_temp_preferred_lft - regen_advance);
+	max_desync_factor = 0;
+	if (cnf_temp_preferred_lft > 0 &&
+	    regen_advance < cnf_temp_preferred_lft)
+		max_desync_factor =
+			min_t(long,
+			      READ_ONCE(idev->cnf.max_desync_factor),
+			      cnf_temp_preferred_lft - regen_advance);
 
 	if (unlikely(idev->desync_factor > max_desync_factor)) {
 		if (max_desync_factor > 0) {
@@ -1485,15 +1501,16 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
 	 * temporary addresses being generated.
 	 */
 	age = (now - tmp_tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ;
-	if (cfg.preferred_lft <= regen_advance + age) {
-		cfg.preferred_lft = regen_advance + age + 1;
-		if (cfg.preferred_lft > cfg.valid_lft ||
-		    cfg.preferred_lft > if_public_preferred_lft) {
+	min_preferred_lft = (u64)regen_advance + age + 1;
+	if (cfg.preferred_lft < min_preferred_lft) {
+		if (min_preferred_lft > cfg.valid_lft ||
+		    min_preferred_lft > if_public_preferred_lft) {
 			in6_ifa_put(ifp);
 			in6_dev_put(idev);
 			ret = -1;
 			goto out;
 		}
+		cfg.preferred_lft = min_preferred_lft;
 	}
 
 	cfg.ifa_flags = IFA_F_TEMPORARY;
@@ -4705,7 +4722,7 @@ static void addrconf_verify_rtnl(struct net *net)
 
 				unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);
 
-				if (age + regen_advance >= ifp->prefered_lft) {
+				if ((u64)age + regen_advance >= ifp->prefered_lft) {
 					struct inet6_ifaddr *ifpub = ifp->ifpub;
 					if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))
 						next = ifp->tstamp + ifp->prefered_lft * HZ;
@@ -6921,7 +6938,8 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.data		= &ipv6_devconf.dad_transmits,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
 	},
 	{
 		.procname	= "router_solicitations",
@@ -7001,14 +7019,16 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.data           = &ipv6_devconf.regen_min_advance,
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
-		.proc_handler   = proc_dointvec,
+		.proc_handler   = proc_dointvec_minmax,
+		.extra1         = SYSCTL_ZERO,
 	},
 	{
 		.procname	= "regen_max_retry",
 		.data		= &ipv6_devconf.regen_max_retry,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
 	},
 	{
 		.procname	= "max_desync_factor",

base-commit: 7076f684d5132e6a6145e1da2fa74771ecd052e5

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

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

> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index c8c0dd04982d9..39ecb2c25055b 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c

[ ... ]

> @@ -4705,7 +4722,7 @@ static void addrconf_verify_rtnl(struct net *net)
>  
>  				unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);
>  
> -				if (age + regen_advance >= ifp->prefered_lft) {
> +				if ((u64)age + regen_advance >= ifp->prefered_lft) {
>  					struct inet6_ifaddr *ifpub = ifp->ifpub;
>  					if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))
>  						next = ifp->tstamp + ifp->prefered_lft * HZ;

[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