* Re: [bug report] net: WireGuard secure network tunnel
[not found] <20200824141519.GA223008@mwanda>
@ 2020-08-24 19:52 ` Jason A. Donenfeld
2020-08-24 20:06 ` [PATCH net] net: read dev->needs_free_netdev before potentially freeing dev Jason A. Donenfeld
1 sibling, 0 replies; 4+ messages in thread
From: Jason A. Donenfeld @ 2020-08-24 19:52 UTC (permalink / raw)
To: Dan Carpenter; +Cc: bpf, Netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH net] net: read dev->needs_free_netdev before potentially freeing dev
[not found] <20200824141519.GA223008@mwanda>
2020-08-24 19:52 ` [bug report] net: WireGuard secure network tunnel Jason A. Donenfeld
@ 2020-08-24 20:06 ` Jason A. Donenfeld
2020-08-24 20:35 ` Jason A. Donenfeld
2020-08-25 8:24 ` Dan Carpenter
1 sibling, 2 replies; 4+ messages in thread
From: Jason A. Donenfeld @ 2020-08-24 20:06 UTC (permalink / raw)
To: netdev; +Cc: Jason A. Donenfeld, Dan Carpenter, David S . Miller
If dev->needs_free_netdev is true, it means that netdev_run_todo should
call free_netdev(dev) after it calls dev->priv_destructor. If
dev->needs_free_netdev is false, then it means that either
dev->priv_destructor is taking care of calling free_netdev(dev), or
something else, elsewhere, is doing that. In this case, branching on
"if (dev->needs_free_netdev)" after calling dev->priv_destructor is a
potential UaF. This patch fixes the issue by reading
dev->needs_free_netdev before calling dev->priv_destructor.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: cf124db566e6 ("net: Fix inconsistent teardown and release of private netdev state.")
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
I believe that the bug Dan reported would easily be fixed as well by
just setting dev->needs_free_netdev=true and removing the call to
free_netdev(dev) in wg_destruct, in wireguard. If you think that this is
the more proper fix -- and that the problem actually isn't this flow in
dev.c and any code that might hit this UaF is wrong -- let me know and
I'll send in a patch for wireguard instead.
net/core/dev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 7df6c9617321..abe53c2fae8c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10073,6 +10073,8 @@ void netdev_run_todo(void)
while (!list_empty(&list)) {
struct net_device *dev
= list_first_entry(&list, struct net_device, todo_list);
+ bool needs_free_netdev = dev->needs_free_netdev;
+
list_del(&dev->todo_list);
if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) {
@@ -10097,7 +10099,7 @@ void netdev_run_todo(void)
#endif
if (dev->priv_destructor)
dev->priv_destructor(dev);
- if (dev->needs_free_netdev)
+ if (needs_free_netdev)
free_netdev(dev);
/* Report a network device has been unregistered */
--
2.28.0
^ permalink raw reply related [flat|nested] 4+ messages in thread