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

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

[Ziyang Xuan]

Add the reference operation to phy_dev of ipvlan to avoid
the potential UAF problem under the following known scenario:

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

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

------
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 v2 1/3] net: ipvlan: fix potential UAF problem for phy_dev

[Ziyang Xuan]

Add the reference operation to phy_dev of ipvlan to avoid
the potential UAF problem under the following known scenario:

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

That likes as the scenario occurred by
commit 563bcbae3ba2 ("net: vlan: fix a UAF in vlan_dev_real_dev()").

Fixes: 2ad7bf363841 ("ipvlan: Initial check-in of the IPVLAN driver.")
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 v2 1/3] net: ipvlan: fix potential UAF problem for phy_dev

[Jakub Kicinski]

On Fri, 18 Mar 2022 09:57:47 +0800 Ziyang Xuan wrote:
> Add the reference operation to phy_dev of ipvlan to avoid
> the potential UAF problem under the following known scenario:
> 
> Someone module puts the NETDEV_UNREGISTER event handler to a
> work, and phy_dev is accessed in the work handler. But when
> the work is excuted, phy_dev has been destroyed because upper
> ipvlan did not get reference to phy_dev correctly.
> 
> That likes as the scenario occurred by
> commit 563bcbae3ba2 ("net: vlan: fix a UAF in vlan_dev_real_dev()").

There is no equivalent of vlan_dev_real_dev() for ipvlan, AFAICT.
The definition of struct ipvl_dev is private to the driver. I don't 
see how a UAF can happen here.

You should either clearly explain how the bug could happen or clearly
state that there is no possibility of the bug for this driver, and the
patch is just future proofing.

If the latter is the case we should drop the Fixes tag and prevent this
patch from getting backported into stable.

> Fixes: 2ad7bf363841 ("ipvlan: Initial check-in of the IPVLAN driver.")
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>

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

[Ziyang Xuan (William)]

> On Fri, 18 Mar 2022 09:57:47 +0800 Ziyang Xuan wrote:
>> Add the reference operation to phy_dev of ipvlan to avoid
>> the potential UAF problem under the following known scenario:
>>
>> Someone module puts the NETDEV_UNREGISTER event handler to a
>> work, and phy_dev is accessed in the work handler. But when
>> the work is excuted, phy_dev has been destroyed because upper
>> ipvlan did not get reference to phy_dev correctly.
>>
>> That likes as the scenario occurred by
>> commit 563bcbae3ba2 ("net: vlan: fix a UAF in vlan_dev_real_dev()").
> 
> There is no equivalent of vlan_dev_real_dev() for ipvlan, AFAICT.
> The definition of struct ipvl_dev is private to the driver. I don't 
> see how a UAF can happen here.
> 
> You should either clearly explain how the bug could happen or clearly
> state that there is no possibility of the bug for this driver, and the
> patch is just future proofing.
> 
> If the latter is the case we should drop the Fixes tag and prevent this
> patch from getting backported into stable.
> 

It is to prevent ipvlan from occurring the similar problem in the future.
For now, there is not way to access phy_dev outside ipvlan module as vlan
real_dev using vlan_dev_real_dev(). I will make this clear.

I think it is necessary to protect lower netdevice using the feature of
netdevice refcnt. So I do it.

Thank you for your valuable opinions! I believe I can do more and more better
with your help.

>> Fixes: 2ad7bf363841 ("ipvlan: Initial check-in of the IPVLAN driver.")
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> .
> 

[PATCH net-next v2 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

[PATCH net-next v2 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