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