| 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: <CAH5Ym4iS=d+dUAg+85wmRJv+jV=Cet=UtN1pNWejMV5fdPVprA@mail.gmail.com>
Date: Thu, 2 Jun 2022 23:02:31 -0600
From: Sam Edwards <cfsworks@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: "David S . Miller" <davem@...emloft.net>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
David Ahern <dsahern@...nel.org>,
Linux Network Development Mailing List
<netdev@...r.kernel.org>
Subject: Re: [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen
Hello,
Since this is a periodic routine, it receives coverage as the system
runs normally. If we're concerned about protecting this change from
regressions and/or through the merge process, some kind of automated
test might be in order, but right now the way to test it is to leave
the system up, with tempaddrs enabled, on a IPv6 SLAAC network, for
several multiples of the temp_prefered_lft. Though the way I see it,
that suggests that a selftest to do just that may be inappropriate
here: one of the rules for selftests is "Don’t take too long."
I can shorten that long line, though do note that my Signed-off-by tag
is correct. That is the canonical capitalization of my email address!
:)
Thanks and see you when net-next is open,
Sam
On Tue, May 31, 2022 at 1:50 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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