netdev - Re: [PATCH net v2] ipv6: Handle race in addrconf_dad

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: <45e62fee-1580-4c5d-7cac-5f0db935fa9e@gmail.com>
Date:   Wed, 2 Oct 2019 16:33:40 -0600
From:   David Ahern <dsahern@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        David Ahern <dsahern@...nel.org>, davem@...emloft.net,
        jakub.kicinski@...ronome.com
Cc:     netdev@...r.kernel.org, rajendra.dendukuri@...adcom.com
Subject: Re: [PATCH net v2] ipv6: Handle race in addrconf_dad_work

On 10/2/19 4:21 PM, Eric Dumazet wrote:
> o syzbot this time, but complete lack of connectivity on some of my test hosts.
> 
> Incoming IPv6 packets go to ip6_forward() (!!!) and are dropped there.

what does 'ip -6 addr sh' show when it is in this state? Any idea of the
order of events?

> 
> There seems to be something missing.
> 
> ifp->state stays at INET6_IFADDR_STATE_PREDAD instead of INET6_IFADDR_STATE_DAD
> 

My original suggestion to Rajendra was this:

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6a576ff92c39..5ec795086432 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4032,6 +4032,12 @@ static void addrconf_dad_work(struct work_struct *w)

        rtnl_lock();

+       /* device was taken down before this delayed work function
+        * could be canceled
+        */
+       if (!(idev->dev->flags & IFF_UP))
+               goto out;
+
        spin_lock_bh(&ifp->lock);
        if (ifp->state == INET6_IFADDR_STATE_PREDAD) {
                action = DAD_BEGIN;


I flipped to IF_READY based on addrconf_ifdown and idev checks seeming
more appropriate.