netdev - Re: [NET 2/2] can: raw: add missing refcount for memory leak fix

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: <03a97ce3-ee82-5cc0-52cd-2501eeebb240@hartkopp.net>
Date: Tue, 22 Aug 2023 18:45:25 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Simon Horman <horms@...nel.org>
Cc: linux-can@...r.kernel.org, netdev@...r.kernel.org, kuba@...nel.org,
 edumazet@...gle.com, mkl@...gutronix.de,
 Ziyang Xuan <william.xuanziyang@...wei.com>
Subject: Re: [NET 2/2] can: raw: add missing refcount for memory leak fix



On 22.08.23 09:59, Simon Horman wrote:
> On Mon, Aug 21, 2023 at 04:45:47PM +0200, Oliver Hartkopp wrote:
>> Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced
>> a new reference to the CAN netdevice that has assigned CAN filters.
>> But this new ro->dev reference did not maintain its own refcount which
>> lead to another KASAN use-after-free splat found by Eric Dumazet.
>>
>> This patch ensures a proper refcount for the CAN nedevice.
>>
>> Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak")
>> Reported-by: Eric Dumazet <edumazet@...gle.com>
>> Cc: Ziyang Xuan <william.xuanziyang@...wei.com>
>> Signed-off-by: Oliver Hartkopp <socketcan@...tkopp.net>
> 
> ...
> 
>> @@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>   		if (!dev) {
>>   			err = -ENODEV;
>>   			goto out;
>>   		}
>>   		if (dev->type != ARPHRD_CAN) {
>> -			dev_put(dev);
>>   			err = -ENODEV;
>> -			goto out;
>> +			goto out_put_dev;
>>   		}
>> +
>>   		if (!(dev->flags & IFF_UP))
>>   			notify_enetdown = 1;
>>   
>>   		ifindex = dev->ifindex;
>>   
>>   		/* filters set by default/setsockopt */
>>   		err = raw_enable_allfilters(sock_net(sk), dev, sk);
>> -		dev_put(dev);
>> +		if (err)
>> +			goto out_put_dev;
>> +
>>   	} else {
>>   		ifindex = 0;
>>   
>>   		/* filters set by default/setsockopt */
>>   		err = raw_enable_allfilters(sock_net(sk), NULL, sk);
>>   	}
>>   
>>   	if (!err) {
>>   		if (ro->bound) {
>>   			/* unregister old filters */
>> -			if (ro->dev)
>> +			if (ro->dev) {
>>   				raw_disable_allfilters(dev_net(ro->dev),
>>   						       ro->dev, sk);
>> -			else
>> +				/* drop reference to old ro->dev */
>> +				netdev_put(ro->dev, &ro->dev_tracker);
>> +			} else {
>>   				raw_disable_allfilters(sock_net(sk), NULL, sk);
>> +			}
>>   		}
>>   		ro->ifindex = ifindex;
>>   		ro->bound = 1;
>> +		/* bind() ok -> hold a reference for new ro->dev */
>>   		ro->dev = dev;
>> +		if (ro->dev)
>> +			netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
>>   	}
>>   
>> - out:
>> +out_put_dev:
>> +	/* remove potential reference from dev_get_by_index() */
>> +	if (dev)
>> +		dev_put(dev);
> 
> Hi Oliver,
> 
> this is possibly not worth a respin, but there is no need to check if dev
> is NULL before calling dev_put(), dev_put() will effectively be a no-op
> with a NULL argument.
> 

Hi Simon,

thanks for your feedback.

In fact I had in mind that someone recently removed some of these "if 
(dev)" statements before "dev_put(dev)" in the netdev subtree.

The reason why I still wanted to point out this check is because of dev 
== NULL is also a valid value for CAN_RAW sockets that are not bound to 
a specific netdev but to 'ALL' CAN netdevs.

So it was more like a documentation purpose than a programming need.

As you don't see a need for a respin too, I can send a patch to can-next 
to remove it, if that fits for you.

Best regards,
Oliver



>> +out:
>>   	release_sock(sk);
>>   	rtnl_unlock();
>>   
>>   	if (notify_enetdown) {
>>   		sk->sk_err = ENETDOWN;
>> -- 
>> 2.39.2
>>
>>
>