On Fri, Aug 4, 2023 at 1:28 PM Oliver Hartkopp <socketcan@xxxxxxxxxxxx> 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.
Hi Olivier
I would feel a bit safer with new longterm device references if you
could add an ro->dev_tracker and use netdev_put() netdev_hold() and
friends ;)
Thanks !
>
> 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>
> ---
> net/can/raw.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/net/can/raw.c b/net/can/raw.c
> index e10f59375659..a61c417fdd27 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -283,12 +283,14 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
>
> switch (msg) {
> case NETDEV_UNREGISTER:
> lock_sock(sk);
> /* remove current filters & unregister */
> - if (ro->bound)
> + if (ro->bound) {
> raw_disable_allfilters(dev_net(dev), dev, sk);
> + dev_put(dev);
> + }
>
> if (ro->count > 1)
> kfree(ro->filter);
>
> ro->ifindex = 0;
> @@ -389,14 +391,16 @@ static int raw_release(struct socket *sock)
> rtnl_lock();
> lock_sock(sk);
>
> /* remove current filters & unregister */
> if (ro->bound) {
> - if (ro->dev)
> + if (ro->dev) {
> raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk);
> - else
> + dev_put(ro->dev);
> + } else {
> raw_disable_allfilters(sock_net(sk), NULL, sk);
> + }
> }
>
> if (ro->count > 1)
> kfree(ro->filter);
>
> @@ -447,36 +451,43 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> if (dev->type != ARPHRD_CAN) {
> dev_put(dev);
> err = -ENODEV;
> goto out;
> }
> - 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) {
> + dev_put(dev);
> + goto out;
> + }
> +
> + ifindex = dev->ifindex;
> + if (!(dev->flags & IFF_UP))
> + notify_enetdown = 1;
> } 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 */
> + dev_put(ro->dev);
> + } else {
> raw_disable_allfilters(sock_net(sk), NULL, sk);
> + }
> }
> ro->ifindex = ifindex;
> ro->bound = 1;
> + /* bind() ok -> holding a reference to new ro->dev */
> ro->dev = dev;
> }
>
> out:
> release_sock(sk);
> --
> 2.39.2
>
