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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 30 Nov 2021 23:30:53 +0200
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     David Ahern <dsahern@...il.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 30/11/2021 19:18, David Ahern wrote:
> 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.
> 

Indeed, that's another way of achieving the same goal.

> 
>>
>>>>
>>>>> 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
> 

Sure, of course. I'd prefer to make the code consistent w.r.t expectations regardless of
the release cycle, but I don't have a strong preference. I'll post the fix with
NULLing the nh_common pointers.

Cheers,
 Nik