netdev - Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after itself on error

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55c48e77-439c-9f0c-f51e-7e944e9b5c1e@gmail.com>
Date:   Tue, 30 Nov 2021 10:18:52 -0700
From:   David Ahern <dsahern@...il.com>
To:     Nikolay Aleksandrov <nikolay@...dia.com>,
        Ido Schimmel <idosch@...sch.org>,
        Nikolay Aleksandrov <razor@...ckwall.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org
Subject: Re: [RFC PATCH net] net: ipv6: make fib6_nh_init properly clean after
 itself on error

On 11/30/21 9:45 AM, Nikolay Aleksandrov wrote:
> On 30/11/2021 18:01, David Ahern wrote:
>> On 11/30/21 5:40 AM, Ido Schimmel wrote:
>>> On Mon, Nov 29, 2021 at 04:11:51PM +0200, Nikolay Aleksandrov wrote:
>>>> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
>>>> index 5dbd4b5505eb..a7debafe8b90 100644
>>>> --- a/net/ipv4/nexthop.c
>>>> +++ b/net/ipv4/nexthop.c
>>>> @@ -2565,14 +2565,8 @@ static int nh_create_ipv6(struct net *net,  struct nexthop *nh,
>>>>  	/* sets nh_dev if successful */
>>>>  	err = ipv6_stub->fib6_nh_init(net, fib6_nh, &fib6_cfg, GFP_KERNEL,
>>>>  				      extack);
>>>> -	if (err) {
>>>> -		/* IPv6 is not enabled, don't call fib6_nh_release */
>>>> -		if (err == -EAFNOSUPPORT)
>>>> -			goto out;
>>>> -		ipv6_stub->fib6_nh_release(fib6_nh);
>>>> -	} else {
>>>> +	if (!err)
>>>>  		nh->nh_flags = fib6_nh->fib_nh_flags;
>>>> -	}
>>>>  out:
>>>>  	return err;
>>>>  }
>>>
>>> This hunk looks good
>>
>> agreed, but it should be a no-op now so this should be a net-next
>> cleanup patch.
>>
> 
> Actually it is needed, it's not a cleanup or noop. If fib6_nh_init fails after fib_nh_common_init
> in the per-cpu allocation then fib6_nh->nh_common's pointers will still be there but
> freed, so it will lead to double free. We have to NULL them when freeing if we want to avoid that.

fib6_nh_init should do proper cleanup if it hits an error. Your bug fix
to get nhc_pcpu_rth_output freed should complete that. It can also set
the value to NULL to avoid double free on any code path.


> 
>>>
>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>>> index 42d60c76d30a..2107b13cc9ab 100644
>>>> --- a/net/ipv6/route.c
>>>> +++ b/net/ipv6/route.c
>>>> @@ -3635,7 +3635,9 @@ int fib6_nh_init(struct net *net, struct fib6_nh *fib6_nh,
>>>>  		in6_dev_put(idev);
>>>>  
>>>>  	if (err) {
>>>> -		lwtstate_put(fib6_nh->fib_nh_lws);
>>>> +		/* check if we failed after fib_nh_common_init() was called */
>>>> +		if (fib6_nh->nh_common.nhc_pcpu_rth_output)
>>>> +			fib_nh_common_release(&fib6_nh->nh_common);
>>>>  		fib6_nh->fib_nh_lws = NULL;
>>>>  		dev_put(dev);
>>>>  	}
>>>
>>> Likewise
>>
>> this is a leak in the current code and should go through -net as a
>> separate patch.
>>
> 
> Yep, this is the point of this patch. :)
> 
>>>
>>>> @@ -3822,7 +3824,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>  	} else {
>>>>  		err = fib6_nh_init(net, rt->fib6_nh, cfg, gfp_flags, extack);
>>>>  		if (err)
>>>> -			goto out;
>>>> +			goto out_free;
>>>>  
>>>>  		fib6_nh = rt->fib6_nh;
>>>>  
>>>> @@ -3841,7 +3843,7 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>  		if (!ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0)) {
>>>>  			NL_SET_ERR_MSG(extack, "Invalid source address");
>>>>  			err = -EINVAL;
>>>> -			goto out;
>>>> +			goto out_free;
>>>>  		}
>>>>  		rt->fib6_prefsrc.addr = cfg->fc_prefsrc;
>>>>  		rt->fib6_prefsrc.plen = 128;
>>>> @@ -3849,12 +3851,13 @@ static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
>>>>  		rt->fib6_prefsrc.plen = 0;
>>>>  
>>>>  	return rt;
>>>> -out:
>>>> -	fib6_info_release(rt);
>>>> -	return ERR_PTR(err);
>>>> +
>>>>  out_free:
>>>>  	ip_fib_metrics_put(rt->fib6_metrics);
>>>> +	if (rt->nh)
>>>> +		nexthop_put(rt->nh);
>>>
>>> Shouldn't this be above ip_fib_metrics_put() given nexthop_get() is
>>> called after ip_fib_metrics_init() ?
>>>
>>> Also, shouldn't we call fib6_nh_release() if fib6_nh_init() succeeded
>>> and we failed later?
>>
>> similarly I think this cleanup is a separate patch.
>>
> 
> Same thing, fib6_info_destroy_rcu -> fib6_nh_release would double-free the nh_common parts
> if fib6_nh_init fails in the per-cpu allocation after fib_nh_common_init.
> It is not a cleanup, but a result of the fix. If we want to keep it, we'll have to NULL
> the nh_common parts when freeing them in fib_nh_common_release().

exactly. set it to NULL and make the -net patch as simple as possible