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

On Mon, Aug 24, 2020 at 4:15 PM Dan Carpenter [off-list ref] 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