netdev - Re: [PATCH] net: check for NULL net

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: <577ABF71.6020002@oracle.com>
Date:	Mon, 4 Jul 2016 21:56:33 +0200
From:	Vegard Nossum <vegard.nossum@...cle.com>
To:	Julian Anastasov <ja@....bg>
Cc:	Andy Gospodarek <gospo@...ulusnetworks.com>, davem@...emloft.net,
	netdev@...r.kernel.org, Dinesh Dutt <ddutt@...ulusnetworks.com>,
	Scott Feldman <sfeldma@...il.com>
Subject: Re: [PATCH] net: check for NULL net_device in FIB tables

On 07/04/2016 09:45 PM, Julian Anastasov wrote:
>
> 	Hello,
>
> On Mon, 4 Jul 2016, Vegard Nossum wrote:
>
>> struct fib_nh->nh_dev can be NULL, so we should check it before calling
>> __in_dev_get_rcu on it.
>>
>> Multiple places seem to want this (and check the return value), so we can
>> add a convenience wrapper for this.
>>
>> This fixes a crash in AF_NETLINK sendmsg().
>>
>> Please double check that I caught all the callers that need the NULL
>> guard.
>
> 	What kind of configuration causes such crash?

It's not very easy to tell, sorry, I was using a netlink fuzzer to find
the crash. But see below, you identified the right bit of code.

>> -		in_dev = __in_dev_get_rtnl(nh->nh_dev);
>> +		in_dev = in_dev_get_rtnl(nh->nh_dev);
>
> 	fib_rebalance can not crash because for multipath
> routes (fib_nhs > 1) all nexthops should have valid nh_dev.
>

Okay, cool.

>>
>>   		if (in_dev &&
>>   		    IN_DEV_IGNORE_ROUTES_WITH_LINKDOWN(in_dev) &&
>> @@ -559,7 +559,7 @@ static void fib_rebalance(struct fib_info *fi)
>>   	change_nexthops(fi) {
>>   		int upper_bound;
>>
>> -		in_dev = __in_dev_get_rtnl(nexthop_nh->nh_dev);
>> +		in_dev = in_dev_get_rtnl(nexthop_nh->nh_dev);
>>
>>   		if (nexthop_nh->nh_flags & RTNH_F_DEAD) {
>>   			upper_bound = -1;
>> @@ -1261,7 +1261,7 @@ int fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
>>   		    nla_put_u32(skb, RTA_OIF, fi->fib_nh->nh_oif))
>>   			goto nla_put_failure;
>>   		if (fi->fib_nh->nh_flags & RTNH_F_LINKDOWN) {
>> -			in_dev = __in_dev_get_rtnl(fi->fib_nh->nh_dev);
>> +			in_dev = in_dev_get_rtnl(fi->fib_nh->nh_dev);
>
> 	Looks like this is the only place that can crash,
> adding extra cycles to the other places is not good. And this
> can happen only because fib_create_info() allows RTNH_F_LINKDOWN
> to come for routes with error. May be fc_flags should be masked
> there? Or there is another place that sets the flag when nh_dev
> is NULL?

Indeed, this is the (only) place that actually crashed for me.

>
>>   			if (in_dev &&
>>   			    IN_DEV_IGNORE_ROUTES_WITH_LINKDOWN(in_dev))
>>   				rtm->rtm_flags |= RTNH_F_DEAD;
>> @@ -1292,7 +1292,7 @@ int fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
>>
>>   			rtnh->rtnh_flags = nh->nh_flags & 0xFF;
>>   			if (nh->nh_flags & RTNH_F_LINKDOWN) {
>> -				in_dev = __in_dev_get_rtnl(nh->nh_dev);
>> +				in_dev = in_dev_get_rtnl(nh->nh_dev);
>
> 	Not needed because fib_nhs > 1
>

Alright.

Thanks for the review! I can submit a new patch to only check the one
place above that actually crashed. Otherwise, if you think it's better
to go with your fc_flags suggestion, feel free to send a patch for that.
As you can tell, I am not very familiar with this code :-) If you do
send a patch, I can test it easily.


Vegard