| 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
| ||
|
Message-ID: <c1c7a1207986d4ad9e80a301fe5e1415631949a9.camel@redhat.com>
Date: Tue, 31 May 2022 09:50:33 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Sam Edwards <cfsworks@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
David Ahern <dsahern@...nel.org>
Cc: Linux Network Development Mailing List <netdev@...r.kernel.org>
Subject: Re: [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen
Hello,
On Fri, 2022-05-27 at 18:48 -0600, Sam Edwards wrote:
> The addrconf_verify_rtnl() function uses a big if/elseif/elseif/... block
> to categorize each address by what type of attention it needs. An
> about-to-expire (RFC 4941) temporary address is one such category, but the
> previous elseif branch catches addresses that have already run out their
> prefered_lft. This means that if addrconf_verify_rtnl() fails to run in
> the necessary time window (i.e. REGEN_ADVANCE time units before the end of
> the prefered_lft), the temporary address will never be regenerated, and no
> temporary addresses will be available until each one's valid_lft runs out
> and manage_tempaddrs() begins anew.
>
> Fix this by moving the entire temporary address regeneration case out of
> that block. That block is supposed to implement the "destructive" part of
> an address's lifecycle, and regenerating a fresh temporary address is not,
> semantically speaking, actually tied to any particular lifecycle stage.
> The age test is also changed from `age >= prefered_lft - regen_advance`
> to `age + regen_advance >= prefered_lft` instead, to ensure no underflow
> occurs if the system administrator increases the regen_advance to a value
> greater than the already-set prefered_lft.
>
> Note that this does not fix the problem of addrconf_verify_rtnl() sometimes
> not running in time, resulting in the race condition described in RFC 4941
> section 3.4 - it only ensures that the address is regenerated. Fixing THAT
> problem may require either using jiffies instead of seconds for all time
> arithmetic here, or always rounding up when regen_advance is converted to
> seconds.
>
> Signed-off-by: Sam Edwards <CFSworks@...il.com>
> ---
> net/ipv6/addrconf.c | 62 ++++++++++++++++++++++++---------------------
> 1 file changed, 33 insertions(+), 29 deletions(-)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index b22504176588..57aa46cb85b7 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4507,6 +4507,39 @@ static void addrconf_verify_rtnl(struct net *net)
> /* We try to batch several events at once. */
> age = (now - ifp->tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ;
>
> + if ((ifp->flags&IFA_F_TEMPORARY) &&
> + !(ifp->flags&IFA_F_TENTATIVE) &&
> + ifp->prefered_lft != INFINITY_LIFE_TIME &&
> + !ifp->regen_count && ifp->ifpub) {
> + /* This is a non-regenerated temporary addr. */
> +
> + unsigned long regen_advance = ifp->idev->cnf.regen_max_retry *
> + ifp->idev->cnf.dad_transmits *
> + max(NEIGH_VAR(ifp->idev->nd_parms, RETRANS_TIME), HZ/100) / HZ;
> +
> + if (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;
> +
> + ifp->regen_count++;
> + in6_ifa_hold(ifp);
> + in6_ifa_hold(ifpub);
> + spin_unlock(&ifp->lock);
> +
> + spin_lock(&ifpub->lock);
> + ifpub->regen_count = 0;
> + spin_unlock(&ifpub->lock);
> + rcu_read_unlock_bh();
> + ipv6_create_tempaddr(ifpub, true);
> + in6_ifa_put(ifpub);
> + in6_ifa_put(ifp);
> + rcu_read_lock_bh();
> + goto restart;
> + } else if (time_before(ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ, next))
> + next = ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ;
> + }
> +
> if (ifp->valid_lft != INFINITY_LIFE_TIME &&
> age >= ifp->valid_lft) {
> spin_unlock(&ifp->lock);
> @@ -4540,35 +4573,6 @@ static void addrconf_verify_rtnl(struct net *net)
> in6_ifa_put(ifp);
> goto restart;
> }
> - } else if ((ifp->flags&IFA_F_TEMPORARY) &&
> - !(ifp->flags&IFA_F_TENTATIVE)) {
> - unsigned long regen_advance = ifp->idev->cnf.regen_max_retry *
> - ifp->idev->cnf.dad_transmits *
> - max(NEIGH_VAR(ifp->idev->nd_parms, RETRANS_TIME), HZ/100) / HZ;
> -
> - if (age >= ifp->prefered_lft - regen_advance) {
> - struct inet6_ifaddr *ifpub = ifp->ifpub;
> - if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))
> - next = ifp->tstamp + ifp->prefered_lft * HZ;
> - if (!ifp->regen_count && ifpub) {
> - ifp->regen_count++;
> - in6_ifa_hold(ifp);
> - in6_ifa_hold(ifpub);
> - spin_unlock(&ifp->lock);
> -
> - spin_lock(&ifpub->lock);
> - ifpub->regen_count = 0;
> - spin_unlock(&ifpub->lock);
> - rcu_read_unlock_bh();
> - ipv6_create_tempaddr(ifpub, true);
> - in6_ifa_put(ifpub);
> - in6_ifa_put(ifp);
> - rcu_read_lock_bh();
> - goto restart;
> - }
> - } else if (time_before(ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ, next))
> - next = ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ;
> - spin_unlock(&ifp->lock);
> } else {
> /* ifp->prefered_lft <= ifp->valid_lft */
> if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))
The change looks correct to me, but it feels potentially
dangerous/impacting currently correct behaviours - especially
considering the lack of selftests for this code-path.
This looks like net-next material, and net-next is currently close. I
suggest to add a self-test verifying the tmp address regeneration and
expiration - I'm not sure how complext that will be, sorry - and re-
post when net-next re-opens.
While at that, please fix your SoB tag (there is a case mismatch with
the sender address) and it would be probably nice to shorten the line
exceeding the 100 chars limit.
Thanks,
Paolo
Powered by blists - more mailing lists