[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

[PATCH net] net: read dev->needs_free_netdev before potentially freeing dev

[Jason A. Donenfeld]

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

Re: [PATCH net] net: read dev->needs_free_netdev before potentially freeing dev

[Jason A. Donenfeld]

On Mon, Aug 24, 2020 at 10:07 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 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.

I think ppp might be hit by the same bug, actually.
netdev_run_todo->ppp_dev_priv_destructor()->ppp_destroy_interface()->free_netdev(dev),
followed by "if (dev->needs_free_netdev)".

Re: [PATCH net] net: read dev->needs_free_netdev before potentially freeing dev

[Dan Carpenter]

On Mon, Aug 24, 2020 at 10:06:50PM +0200, Jason A. Donenfeld wrote:
> 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.
> 

No, I misread the code.  Sorry.  This patch is not required.  We can
use "dev" up to the end of the function where we do:

		/* Free network device */
		kobject_put(&dev->dev.kobj);

That's where the final reference is released.

regards,
dan carpenter