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.
> +out:
> release_sock(sk);
> rtnl_unlock();
>
> if (notify_enetdown) {
> sk->sk_err = ENETDOWN;
> --
> 2.39.2
>
>
