linux-kernel - Re: [PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr is removed or un-mngtmpaddr

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: <CAH5Ym4jjVFofG5J7QW=EsD00siDXtNWKt4ZDNbbUmP+Y4Jb-DQ@mail.gmail.com>
Date: Wed, 13 Nov 2024 13:03:13 -0800
From: Sam Edwards <cfsworks@...il.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	David Ahern <dsahern@...nel.org>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net 1/2] net/ipv6: delete temporary address if mngtmpaddr
 is removed or un-mngtmpaddr

On Wed, Nov 13, 2024 at 4:52 AM Hangbin Liu <liuhangbin@...il.com> wrote:
>
> RFC8981 section 3.4 says that existing temporary addresses must have their
> lifetimes adjusted so that no temporary addresses should ever remain "valid"
> or "preferred" longer than the incoming SLAAC Prefix Information. This would
> strongly imply in Linux's case that if the "mngtmpaddr" address is deleted or
> un-flagged as such, its corresponding temporary addresses must be cleared out
> right away.
>
> But now the temporary address is renewed even after ‘mngtmpaddr’ is removed
> or becomes unmanaged. Fix this by deleting the temporary address with this
> situation.

Hi Hangbin,

Is it actually a new temporary, or is it just failing to purge the old
temporaries? While trying to understand this bug on my own, I learned
about a commit [1] that tried to address the former problem, and it's
possible that the approach in that commit needs to be tightened up
instead.

[1]: 69172f0bcb6a09 ("ipv6 addrconf: fix bug where deleting a
mngtmpaddr can create a new temporary address")

>
> Fixes: 778964f2fdf0 ("ipv6/addrconf: fix timing bug in tempaddr regen")
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>  net/ipv6/addrconf.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 94dceac52884..6852dbce5a7d 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4644,6 +4644,10 @@ static void addrconf_verify_rtnl(struct net *net)
>                             !ifp->regen_count && ifp->ifpub) {
>                                 /* This is a non-regenerated temporary addr. */
>
> +                               if ((!ifp->valid_lft && !ifp->prefered_lft) ||
> +                                   ifp->ifpub->state == INET6_IFADDR_STATE_DEAD)
> +                                       goto delete_ifp;
> +

My understanding is that the kernel already calls
`manage_tempaddrs(dev, ifp, 0, 0, false, now);` when some `ifp` needs
its temporaries flushed out. That zeroes out the lifetimes of every
temporary, which allows the "destructive" if/elseif/elseif/... block
below to delete it. I believe fixing this bug properly will involve
first understanding why *that* mechanism isn't working as designed.

In any case, this 'if' block is for temporary addresses /which haven't
yet begun their regeneration process/, so this won't work to purge out
addresses that have already started trying to create their
replacement. That'll be a rare and frustrating race for someone in the
future to debug indeed. As well, I broke this 'if' out from the below
if/elseif/elseif/... to establish a clear separation between the
"constructive" parts of an address's lifecycle (currently, only
temporary addresses needing to regenerate) and the "destructive" parts
(the address gradually becoming less prominent as its lifetime runs
out), so destructive/delete logic doesn't belong up here anyway.

What are your thoughts?

Happy Wednesday,
Sam

>                                 unsigned long regen_advance = ipv6_get_regen_advance(ifp->idev);
>
>                                 if (age + regen_advance >= ifp->prefered_lft) {
> @@ -4671,6 +4675,7 @@ static void addrconf_verify_rtnl(struct net *net)
>
>                         if (ifp->valid_lft != INFINITY_LIFE_TIME &&
>                             age >= ifp->valid_lft) {
> +delete_ifp:
>                                 spin_unlock(&ifp->lock);
>                                 in6_ifa_hold(ifp);
>                                 rcu_read_unlock_bh();
> --
> 2.46.0
>