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@xxxxxxxxxx>
> > > Cc: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx>
> > > Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
> >
> > ...
> >
> > > @@ -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.
