netdev - Re: [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath

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: <26bd52c0-1218-3c65-95ca-b2a908466841@cumulusnetworks.com>
Date:   Sun, 3 Jun 2018 08:46:44 -0600
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: ipv6: prevent use after free in
 ip6_route_mpath_notify()

On 6/3/18 8:31 AM, Eric Dumazet wrote:
> 
> 
> On 06/03/2018 07:01 AM, David Ahern wrote:
>> On 6/3/18 7:35 AM, Eric Dumazet wrote:
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
>>>  
>>>  	err_nh = NULL;
>>>  	list_for_each_entry(nh, &rt6_nh_list, next) {
>>> +		dst_release(&rt_last->dst);
>>>  		rt_last = nh->rt6_info;
>>> +		dst_hold(&rt_last->dst);
>>>  		err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
>>>  		/* save reference to first route for notification */
>>>  		if (!rt_notif && !err)
>>> @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
>>>  		list_del(&nh->next);
>>>  		kfree(nh);
>>>  	}
>>> -
>>> +	dst_release(&rt_last->dst);
>>>  	return err;
>>>  }
>>
>> Since the rtnl lock is held, a successfully inserted route can not be
>> removed until ip6_route_multipath_add finishes. This is a simpler change
>> that works with net-next as well:
> 
> Your patch changes the intent of your original commit.
> 
> It seems you wanted rt_last to point to the last attempted insertion,
> not the last successful one ?

The note in ip6_route_mpath_notify explains it:

        /* if this is an APPEND route, then rt points to the first route
         * inserted and rt_last points to last route inserted. Userspace

> 
> Or have I misunderstood, and not only we had a use-after-free, but also
> a semantic error ?

It was a mistake to set rt_last before checking err. So the
use-after-free exposed the semantic error.