netdev - Re: [PATCH net-next 3/3] net: ipv6/addrconf: clamp preferred

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <89281fb2-fb5d-416a-aff9-31cf6a0d4525@kernel.org>
Date: Tue, 13 Feb 2024 08:40:42 -0700
From: David Ahern <dsahern@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: edumazet@...gle.com, kuba@...nel.org, jikos@...nel.org,
 Alex Henrie <alexhenrie24@...il.com>, netdev@...r.kernel.org, dan@...m.net,
 bagasdotme@...il.com, davem@...emloft.net
Subject: Re: [PATCH net-next 3/3] net: ipv6/addrconf: clamp preferred_lft to
 the minimum required

On 2/13/24 3:13 AM, Paolo Abeni wrote:
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 0b78ffc101ef..8d3023e54822 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -1347,6 +1347,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
>>  	unsigned long regen_advance;
>>  	unsigned long now = jiffies;
>>  	s32 cnf_temp_preferred_lft;
>> +	u32 if_public_preferred_lft;
> 
> [only if a repost is needed for some other reason] please respect the
> reverse x-mas tree above.
> 
>>  	struct inet6_ifaddr *ift;
>>  	struct ifa6_config cfg;
>>  	long max_desync_factor;
>> @@ -1401,11 +1402,13 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
>>  		}
>>  	}
>>  
>> +	if_public_preferred_lft = ifp->prefered_lft;
>> +
>>  	memset(&cfg, 0, sizeof(cfg));
>>  	cfg.valid_lft = min_t(__u32, ifp->valid_lft,
>>  			      idev->cnf.temp_valid_lft + age);
>>  	cfg.preferred_lft = cnf_temp_preferred_lft + age - idev->desync_factor;
>> -	cfg.preferred_lft = min_t(__u32, ifp->prefered_lft, cfg.preferred_lft);
>> +	cfg.preferred_lft = min_t(__u32, if_public_preferred_lft, cfg.preferred_lft);
>>  	cfg.preferred_lft = min_t(__u32, cfg.valid_lft, cfg.preferred_lft);
>>  
>>  	cfg.plen = ifp->prefix_len;
>> @@ -1414,19 +1417,41 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
>>  
>>  	write_unlock_bh(&idev->lock);
>>  
>> -	/* A temporary address is created only if this calculated Preferred
>> -	 * Lifetime is greater than REGEN_ADVANCE time units.  In particular,
>> -	 * an implementation must not create a temporary address with a zero
>> -	 * Preferred Lifetime.
>> +	/* From RFC 4941:
>> +	 *
>> +	 *     A temporary address is created only if this calculated Preferred
>> +	 *     Lifetime is greater than REGEN_ADVANCE time units.  In
>> +	 *     particular, an implementation must not create a temporary address
>> +	 *     with a zero Preferred Lifetime.
>> +	 *
>> +	 *     ...
>> +	 *
>> +	 *     When creating a temporary address, the lifetime values MUST be
>> +	 *     derived from the corresponding prefix as follows:
>> +	 *
>> +	 *     ...
>> +	 *
>> +	 *     *  Its Preferred Lifetime is the lower of the Preferred Lifetime
>> +	 *        of the public address or TEMP_PREFERRED_LIFETIME -
>> +	 *        DESYNC_FACTOR.
>> +	 *
>> +	 * To comply with the RFC's requirements, clamp the preferred lifetime
>> +	 * to a minimum of regen_advance, unless that would exceed valid_lft or
>> +	 * ifp->prefered_lft.
>> +	 *
>>  	 * Use age calculation as in addrconf_verify to avoid unnecessary
>>  	 * temporary addresses being generated.
>>  	 */
>>  	age = (now - tmp_tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ;
>>  	if (cfg.preferred_lft <= regen_advance + age) {
>> -		in6_ifa_put(ifp);
>> -		in6_dev_put(idev);
>> -		ret = -1;
>> -		goto out;
>> +		cfg.preferred_lft = regen_advance + age + 1;
>> +		if (cfg.preferred_lft > cfg.valid_lft ||
>> +		    cfg.preferred_lft > if_public_preferred_lft) {
>> +			in6_ifa_put(ifp);
>> +			in6_dev_put(idev);
>> +			ret = -1;
>> +			goto out;
>> +		}
>>  	}
>>  
>>  	cfg.ifa_flags = IFA_F_TEMPORARY;
> 
> The above sounds reasonable to me, but I would appreciate a couple of
> additional eyeballs on it. @David, could you please have a look?
> 

I went through the set this past weekend along with the earlier thread
with the problem Dan mentioned. I think the logic is ok. Dan: did you
get a chance to test this set?

Reviewed-by: David Ahern <dsahern@...nel.org>