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

[Simon Horman <horms@kernel.org>]

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@google.com>
> Cc: Ziyang Xuan <william.xuanziyang@huawei.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.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.

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