netdev - Re: [PATCH] net: ipv6: Fix warning of freeing alive inet6 address

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: <f52bbfcd-e961-190a-6aa5-3f43d833f27c@brocade.com>
Date:   Wed, 3 May 2017 19:24:20 +0100
From:   Mike Manning <mmanning@...cade.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
CC:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: ipv6: Fix warning of freeing alive inet6 address

On 03/05/17 18:58, Cong Wang wrote:
> On Tue, May 2, 2017 at 11:30 AM, Mike Manning <mmanning@...cade.com> wrote:
>> While this is not reproducible manually, Andrey's syzkaller program hit
>> the warning "IPv6: Freeing alive inet6 address" with this part trace:
>>
>> inet6_ifa_finish_destroy+0x12e/0x190 c:894
>> in6_ifa_put ./include/net/addrconf.h:330
>> addrconf_dad_work+0x4e9/0x1040 net/ipv6/addrconf.c:3963
>>
>> The fix is to call in6_ifa_put() for the inet6_ifaddr before rather
>> than after calling addrconf_ifdown(), as the latter may remove it from
>> the address hash table.
>>
>> Fixes: 85b51b12115c ("net: ipv6: Remove addresses for failures with strict DAD")
>> Reported-by: Andrey Konovalov <andreyknvl@...gle.com>
>> Signed-off-by: Mike Manning <mmanning@...cade.com>
>> ---
>>  net/ipv6/addrconf.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 80ce478..361993a 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -3902,8 +3902,11 @@ static void addrconf_dad_work(struct work_struct *w)
>>         } else if (action == DAD_ABORT) {
>>                 in6_ifa_hold(ifp);
>>                 addrconf_dad_stop(ifp, 1);
>> -               if (disable_ipv6)
>> +               if (disable_ipv6) {
>> +                       in6_ifa_put(ifp);
>>                         addrconf_ifdown(idev->dev, 0);
>> +                       goto unlock;
>> +               }
> 
> 
> But addrconf_dad_stop() calls ipv6_del_addr() which could unhash
> the addr too...
> 

Agreed, and in the mean time Andrey has confirmed that this v1 patch
does not resolve the issue. The problem is not specific to my change
for removing addresses. It seems that generally here the in6_ifa_hold()
and matching in6_ifa_put() are surplus to requirement, as the address
refcnt is 2 even without the hold before calling DAD stop.