netdev - Re: [PATCH] ipv6/addrconf: clamp preferred_lft to the minimum instead of erroring

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: <17afabe7bc302ac5d0cac87410ba9fa0eda144b3.camel@redhat.com>
Date: Wed, 23 Aug 2023 13:00:03 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Alex Henrie <alexhenrie24@...il.com>
Cc: netdev@...r.kernel.org, jbohac@...e.cz, benoit.boissinot@...-lyon.org, 
	davem@...emloft.net, hideaki.yoshifuji@...aclelinux.com, dsahern@...nel.org
Subject: Re: [PATCH] ipv6/addrconf: clamp preferred_lft to the minimum
 instead of erroring

On Tue, 2023-08-22 at 21:41 -0600, Alex Henrie wrote:
> On Tue, Aug 22, 2023 at 3:54 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > On Sun, 2023-08-20 at 19:11 -0600, Alex Henrie wrote:
> 
> > > @@ -1368,7 +1368,7 @@ 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(__u32,
> > > +     max_desync_factor = min_t(__s64,
> > >                                 idev->cnf.max_desync_factor,
> > >                                 cnf_temp_preferred_lft - regen_advance);
> > 
> > It would be better if you describe in the commit message your above
> > fix.
> 
> I did mention the underflow problem in the commit message. When I
> split the patch into two patches, it will be even more prominent. What
> more would you like the commit message to say?

I think explicitly mentioning that the existing code incorrectly casted
a negative value to an unsigned one should suffice. 

> 
> > Also possibly using 'long' as the target type (same as
> > 'max_desync_factor') would be more clear.
> 
> OK, will change in v2.
> 
> > > @@ -1402,12 +1402,8 @@ 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) {
> > > -             in6_ifa_put(ifp);
> > > -             in6_dev_put(idev);
> > > -             ret = -1;
> > > -             goto out;
> > > -     }
> > > +     if (cfg.preferred_lft <= regen_advance + age)
> > > +             cfg.preferred_lft = regen_advance + age + 1;
> > 
> > This change obsoletes the comment pairing the code. At very least you
> > should update that and the sysctl knob description in
> > Documentation/networking/ip-sysctl.rst.
> 
> The general idea is still valid: The preferred lifetime must be
> greater than regen_advance. I will rephrase the comment to be more
> clear in v2.
> 
> > But I'm unsure we can raise the preferred lifetime so easily. e.g. what
> > if preferred_lft becomes greater then valid_lft?
> 
> Excellent point. We really should clamp preferred_lft to valid_lft as
> well. I can make that change in v2.
> 
> By the way, if valid_lft is less than regen_advance, temporary
> addresses still won't work. However, that is much more understandable
> because valid_lft has to be at least the length of the longest needed
> connection, so in practice it's always going to be much longer than 5
> seconds.
> 
> > I think a fairly safer alternative option would be documenting the
> > current behavior in ip-sysctl.rst
> 
> I feel strongly that the current behavior, which can appear to be
> working fine for a few minutes before breaking, is very undesirable.
> I
> could, nonetheless, add some explanation to ip-sysctl.rst about what
> happens if preferred_lft or valid_lft is too small.

I think that we could accept the general idea that setting some
"extreme"/edge values on system settings will lead to unexpected
results/limited functionality.

IDK how much relevant is the 'preferred_lft < 5' use-case.

I fear that changing "under-the-hood" the preferred lifetime value in
use could have unexpected side effects for other scenarios. e.g. we can
hit the 'increase preferred lifetime' condition even when:

cfg.preferred_lft == <some largish, more common, value>
age == ~cfg.preferred_lft

@David A.: I would love to hear your opinion here.

Thank,

Paolo