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: <CAMMLpeRR_JmFp3DnDJbYdjxnpfxLke-z5KW=EA8_H_xj3FzXvg@mail.gmail.com>
Date: Tue, 22 Aug 2023 21:41:37 -0600
From: Alex Henrie <alexhenrie24@...il.com>
To: Paolo Abeni <pabeni@...hat.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

Hi Paolo, thanks for the review.

On Tue, Aug 22, 2023 at 3:54 AM Paolo Abeni <pabeni@...hat.com> wrote:

> It looks like you are fixing 2 separate bugs, so 2 separate patches
> would be better.

The two problems are closely related, and in the same function. But I
will split the patch into two patches to your preference.

> You should explicitly state the target tree (in this case 'net') into
> the patch subj.

Will fix in v2, thanks.

> You should add a suitable fixes tag to each patch.

That would be "Fixes: 76506a986dc31394fd1f2741db037d29c7e57843" and
"Fixes: eac55bf97094f6b64116426864cf4666ef7587bc", correct?

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

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

Thanks for caring about doing IPv6 right,

-Alex