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

[Simon Horman <horms@kernel.org>]

On Tue, Aug 22, 2023 at 06:45:25PM +0200, Oliver Hartkopp wrote:
> 
> 
> 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@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.
> > 
> 
> 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.

Thanks Oliver, that would be fine by me.