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

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

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 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()").

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

[PATCH net-next 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 3/3] net: ipvtap: fix error comments

[Ziyang Xuan]

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

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

[Eric Dumazet]

On Mon, Mar 14, 2022 at 3:54 AM Ziyang Xuan
<william.xuanziyang@huawei.com> 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.

Can you name the module deferring NETDEV_UNREGISTER to a work queue ?

This sounds like a bug to me.

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

Mentioning a commit that added a bug and many other commits trying to
fix it is a bit unfortunate.

Can you instead add a Fixes: tag ?

Do you have a repro to trigger the bug ?

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

[Ziyang Xuan (William)]

> On Mon, Mar 14, 2022 at 3:54 AM Ziyang Xuan
> <william.xuanziyang@huawei.com> 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.
> 
> Can you name the module deferring NETDEV_UNREGISTER to a work queue ?

The one I know is nb_netdevice netdevice notifier of roce_gid_mgmt.
It will trigger vlan's real_dev UAF. It is a syzbot problem.
See details:
https://syzkaller.appspot.com/bug?extid=e4df4e1389e28972e955

> 
> This sounds like a bug to me.
> 
>>
>> That likes as the scenario occurred by
>> commit 563bcbae3ba2 ("net: vlan: fix a UAF in vlan_dev_real_dev()").
> 
> Mentioning a commit that added a bug and many other commits trying to
> fix it is a bit unfortunate.

Yes, I am sorry for that. I will keep improving the quality of my code.

The related problems have solved by the series patches of
commit faab39f63c1f ("net: allow out-of-order netdev unregistration").
So I think it is the right time to fix other modules' potential problem.

> 
> Can you instead add a Fixes: tag ?

The potential problem exists since macvlan and ipvlan were added.

> 
> Do you have a repro to trigger the bug ?

For macvlan and ipvlan, there are not repros now. I think it is necessary
to give a right logic to cope with the constant evolution of the kernel.

> 
>>
>> 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
>>
> .
>