[bug report] net: WireGuard secure network tunnel

[bug report] net: WireGuard secure network tunnel

[Dan Carpenter]

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

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

[Jason A. Donenfeld]

On Mon, Aug 24, 2020 at 4:15 PM Dan Carpenter <dan.carpenter@oracle.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