[PATCH net-next v3 0/3] net: ipvlan: fix potential UAF problem for phy_dev

[PATCH net-next v3 0/3] net: ipvlan: fix potential UAF problem for phy_dev

[Ziyang Xuan]

There is a known scenario can trigger UAF problem for lower
netdevice as following:

Someone module puts the NETDEV_UNREGISTER event handler to a
work, and lower netdevice is accessed in the work handler. But
when the work is excuted, lower netdevice has been destroyed
because upper netdevice did not get reference to lower netdevice
correctly.

Although it can not happen for ipvlan now because there is no
way to access phy_dev outside ipvlan. But it is necessary to
add the reference operation to phy_dev to avoid the potential
UAF problem in the future.

In addition, add net device refcount tracker to ipvlan and
fix some error comments for ipvtap module.

---
v2->v3:
  - Make it clear that the problem can not happen now but for future.
  - Delete "Fixes: tag" to avoid backporting to stable.
v1->v2:
  - Add "Fixes: tag" for fix patches.

Ziyang Xuan (3):
  net: ipvlan: fix potential UAF problem for phy_dev
  net: ipvlan: add net device refcount tracker
  net: ipvtap: fix error comments

 drivers/net/ipvlan/ipvlan.h      |  1 +
 drivers/net/ipvlan/ipvlan_main.c | 13 +++++++++++++
 drivers/net/ipvlan/ipvtap.c      |  4 ++--
 3 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH net-next v3 1/3] net: ipvlan: fix potential UAF problem for phy_dev

[Ziyang Xuan]

There is a known scenario can trigger UAF problem for lower
netdevice as following:

Someone module puts the NETDEV_UNREGISTER event handler to a
work, and lower netdevice is accessed in the work handler. But
when the work is excuted, lower netdevice has been destroyed
because upper netdevice did not get reference to lower netdevice
correctly.

Although it can not happen for ipvlan now because there is no
way to access phy_dev outside ipvlan. But it is necessary to
add the reference operation to phy_dev to avoid the potential
UAF problem in the future.

Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 drivers/net/ipvlan/ipvlan_main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 696e245f6d00..dcdc01403f22 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -158,6 +158,10 @@ static int ipvlan_init(struct net_device *dev)
 	}
 	port = ipvlan_port_get_rtnl(phy_dev);
 	port->count += 1;
+
+	/* Get ipvlan's reference to phy_dev */
+	dev_hold(phy_dev);
+
 	return 0;
 }
 
@@ -665,6 +669,14 @@ void ipvlan_link_delete(struct net_device *dev, struct list_head *head)
 }
 EXPORT_SYMBOL_GPL(ipvlan_link_delete);
 
+static void ipvlan_dev_free(struct net_device *dev)
+{
+	struct ipvl_dev *ipvlan = netdev_priv(dev);
+
+	/* Get rid of the ipvlan's reference to phy_dev */
+	dev_put(ipvlan->phy_dev);
+}
+
 void ipvlan_link_setup(struct net_device *dev)
 {
 	ether_setup(dev);
@@ -674,6 +686,7 @@ void ipvlan_link_setup(struct net_device *dev)
 	dev->priv_flags |= IFF_UNICAST_FLT | IFF_NO_QUEUE;
 	dev->netdev_ops = &ipvlan_netdev_ops;
 	dev->needs_free_netdev = true;
+	dev->priv_destructor = ipvlan_dev_free;
 	dev->header_ops = &ipvlan_header_ops;
 	dev->ethtool_ops = &ipvlan_ethtool_ops;
 }
-- 
2.25.1

Re: [PATCH net-next v3 1/3] net: ipvlan: fix potential UAF problem for phy_dev

[Eric Dumazet]

On 3/19/22 02:52, Ziyang Xuan wrote:
> There is a known scenario can trigger UAF problem for lower
> netdevice as following:


But this known scenario never happens.

So maybe you should not add it 'in the future'


>
> Someone module puts the NETDEV_UNREGISTER event handler to a
> work, and lower netdevice is accessed in the work handler. But
> when the work is excuted, lower netdevice has been destroyed
> because upper netdevice did not get reference to lower netdevice
> correctly.

Again, whoever would like to use a work queue would also

need to hold a reference on the device.

Regardless of ipvlan being used or not.


>
> Although it can not happen for ipvlan now because there is no
> way to access phy_dev outside ipvlan. But it is necessary to
> add the reference operation to phy_dev to avoid the potential
> UAF problem in the future.
>
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---


Your patch makes no sense to me really.

[PATCH net-next v3 2/3] net: ipvlan: add net device refcount tracker

[Ziyang Xuan]

Add net device refcount tracker to ipvlan.

Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 drivers/net/ipvlan/ipvlan.h      | 1 +
 drivers/net/ipvlan/ipvlan_main.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h
index 3837c897832e..6605199305b7 100644
--- a/drivers/net/ipvlan/ipvlan.h
+++ b/drivers/net/ipvlan/ipvlan.h
@@ -64,6 +64,7 @@ struct ipvl_dev {
 	struct list_head	pnode;
 	struct ipvl_port	*port;
 	struct net_device	*phy_dev;
+	netdevice_tracker	dev_tracker;
 	struct list_head	addrs;
 	struct ipvl_pcpu_stats	__percpu *pcpu_stats;
 	DECLARE_BITMAP(mac_filters, IPVLAN_MAC_FILTER_SIZE);
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index dcdc01403f22..be06f122092e 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -160,7 +160,7 @@ static int ipvlan_init(struct net_device *dev)
 	port->count += 1;
 
 	/* Get ipvlan's reference to phy_dev */
-	dev_hold(phy_dev);
+	dev_hold_track(phy_dev, &ipvlan->dev_tracker, GFP_KERNEL);
 
 	return 0;
 }
@@ -674,7 +674,7 @@ static void ipvlan_dev_free(struct net_device *dev)
 	struct ipvl_dev *ipvlan = netdev_priv(dev);
 
 	/* Get rid of the ipvlan's reference to phy_dev */
-	dev_put(ipvlan->phy_dev);
+	dev_put_track(ipvlan->phy_dev, &ipvlan->dev_tracker);
 }
 
 void ipvlan_link_setup(struct net_device *dev)
-- 
2.25.1

Re: [PATCH net-next v3 2/3] net: ipvlan: add net device refcount tracker

[Eric Dumazet]

On 3/19/22 02:53, Ziyang Xuan wrote:
> Add net device refcount tracker to ipvlan.


Just squash this to prior patch.


Ideally all new dev_hold()/dev_put() should be

dev_hold_track() and dev_put_track()


> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
>   drivers/net/ipvlan/ipvlan.h      | 1 +
>   drivers/net/ipvlan/ipvlan_main.c | 4 ++--
>   2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h
> index 3837c897832e..6605199305b7 100644
> --- a/drivers/net/ipvlan/ipvlan.h
> +++ b/drivers/net/ipvlan/ipvlan.h
> @@ -64,6 +64,7 @@ struct ipvl_dev {
>   	struct list_head	pnode;
>   	struct ipvl_port	*port;
>   	struct net_device	*phy_dev;
> +	netdevice_tracker	dev_tracker;
>   	struct list_head	addrs;
>   	struct ipvl_pcpu_stats	__percpu *pcpu_stats;
>   	DECLARE_BITMAP(mac_filters, IPVLAN_MAC_FILTER_SIZE);
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index dcdc01403f22..be06f122092e 100644
> --- a/drivers/net/ipvlan/ipvlan_main.c
> +++ b/drivers/net/ipvlan/ipvlan_main.c
> @@ -160,7 +160,7 @@ static int ipvlan_init(struct net_device *dev)
>   	port->count += 1;
>   
>   	/* Get ipvlan's reference to phy_dev */
> -	dev_hold(phy_dev);
> +	dev_hold_track(phy_dev, &ipvlan->dev_tracker, GFP_KERNEL);
>   
>   	return 0;
>   }
> @@ -674,7 +674,7 @@ static void ipvlan_dev_free(struct net_device *dev)
>   	struct ipvl_dev *ipvlan = netdev_priv(dev);
>   
>   	/* Get rid of the ipvlan's reference to phy_dev */
> -	dev_put(ipvlan->phy_dev);
> +	dev_put_track(ipvlan->phy_dev, &ipvlan->dev_tracker);
>   }
>   
>   void ipvlan_link_setup(struct net_device *dev)

[PATCH net-next v3 3/3] net: ipvtap: fix error comments

[Ziyang Xuan]

Use "macvlan" comment inappropriately in ipvtap module.
Fix them with "ipvlan" comment.

Fixes: 235a9d89da97 ("ipvtap: IP-VLAN based tap driver")
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 drivers/net/ipvlan/ipvtap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipvlan/ipvtap.c b/drivers/net/ipvlan/ipvtap.c
index ef02f2cf5ce1..c130cfb30822 100644
--- a/drivers/net/ipvlan/ipvtap.c
+++ b/drivers/net/ipvlan/ipvtap.c
@@ -83,7 +83,7 @@ static int ipvtap_newlink(struct net *src_net, struct net_device *dev,
 
 	INIT_LIST_HEAD(&vlantap->tap.queue_list);
 
-	/* Since macvlan supports all offloads by default, make
+	/* Since ipvlan supports all offloads by default, make
 	 * tap support all offloads also.
 	 */
 	vlantap->tap.tap_features = TUN_OFFLOADS;
@@ -95,7 +95,7 @@ static int ipvtap_newlink(struct net *src_net, struct net_device *dev,
 	if (err)
 		return err;
 
-	/* Don't put anything that may fail after macvlan_common_newlink
+	/* Don't put anything that may fail after ipvlan_link_new
 	 * because we can't undo what it does.
 	 */
 	err =  ipvlan_link_new(src_net, dev, tb, data, extack);
-- 
2.25.1

Re: [PATCH net-next v3 0/3] net: ipvlan: fix potential UAF problem for phy_dev

[Paolo Abeni]

Hello,

On Sat, 2022-03-19 at 17:52 +0800, Ziyang Xuan wrote:
> There is a known scenario can trigger UAF problem for lower
> netdevice as following:
> 
> Someone module puts the NETDEV_UNREGISTER event handler to a
> work, and lower netdevice is accessed in the work handler. But
> when the work is excuted, lower netdevice has been destroyed
> because upper netdevice did not get reference to lower netdevice
> correctly.
> 
> Although it can not happen for ipvlan now because there is no
> way to access phy_dev outside ipvlan. But it is necessary to
> add the reference operation to phy_dev to avoid the potential
> UAF problem in the future.
> 
> In addition, add net device refcount tracker to ipvlan and
> fix some error comments for ipvtap module.

This is pure net-next material, and we are now into the merge window -
only fixes allowed. Please repost in 2w, thanks!

Paolo