netdev - Re: [bug report] net: WireGuard secure network tunnel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [day] [month] [year] [list]

Date:   Mon, 24 Aug 2020 21:52:45 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     bpf@...r.kernel.org, Netdev <netdev@...r.kernel.org>
Subject: Re: [bug report] net: WireGuard secure network tunnel

On Mon, Aug 24, 2020 at 4:15 PM Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
> Hello Jason A. Donenfeld,
>
> The patch e7096c131e51: "net: WireGuard secure network tunnel" from
> Dec 9, 2019, leads to the following static checker warning:
>
>         net/core/dev.c:10103 netdev_run_todo()
>         warn: 'dev->_tx' double freed
>
> net/core/dev.c
>  10071          /* Wait for rcu callbacks to finish before next phase */
>  10072          if (!list_empty(&list))
>  10073                  rcu_barrier();
>  10074
>  10075          while (!list_empty(&list)) {
>  10076                  struct net_device *dev
>  10077                          = list_first_entry(&list, struct net_device, todo_list);
>  10078                  list_del(&dev->todo_list);
>  10079
>  10080                  if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) {
>  10081                          pr_err("network todo '%s' but state %d\n",
>  10082                                 dev->name, dev->reg_state);
>  10083                          dump_stack();
>  10084                          continue;
>  10085                  }
>  10086
>  10087                  dev->reg_state = NETREG_UNREGISTERED;
>  10088
>  10089                  netdev_wait_allrefs(dev);
>  10090
>  10091                  /* paranoia */
>  10092                  BUG_ON(netdev_refcnt_read(dev));
>  10093                  BUG_ON(!list_empty(&dev->ptype_all));
>  10094                  BUG_ON(!list_empty(&dev->ptype_specific));
>  10095                  WARN_ON(rcu_access_pointer(dev->ip_ptr));
>  10096                  WARN_ON(rcu_access_pointer(dev->ip6_ptr));
>  10097  #if IS_ENABLED(CONFIG_DECNET)
>  10098                  WARN_ON(dev->dn_ptr);
>  10099  #endif
>  10100                  if (dev->priv_destructor)
>  10101                          dev->priv_destructor(dev);
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^
> The wg_destruct() functions frees "dev".
>
>  10102                  if (dev->needs_free_netdev)
>                             ^^^^^
> Use after free.
>
>  10103                          free_netdev(dev);
>  10104
>  10105                  /* Report a network device has been unregistered */
>  10106                  rtnl_lock();
>  10107                  dev_net(dev)->dev_unreg_count--;
>  10108                  __rtnl_unlock();
>  10109                  wake_up(&netdev_unregistering_wq);
>  10110
>  10111                  /* Free network device */
>  10112                  kobject_put(&dev->dev.kobj);
>  10113          }
>  10114  }
>
> regards,
> dan carpenter

I actually recall a patch ~3 years ago from DaveM trying to make
netdev tear down semantics a bit cleaner, by distinguishing between
the case when netdevs free their own dev and when they don't. I'm not
sure whether wireguard should set needs_free_netdev or not, but I
vaguely remember reasoning about that a long time ago and deciding,
"no". However, branching on dev->needs_free_netdev seems like it must
be a UaF always in the case where needs_free_netdev is false, since in
that case, the destructor should free it (I think). I'll send a patch,
and see if DaveM likes that, or if he'd prefer I just set
needs_free_netdev in wireguard and remove the free_netdev call in
wg_destruct.

Thanks for the report.

Jason