| Message ID | 83116bde1ddf39420e24466684c9488bff46f43c.1647568181.git.william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipvlan: fix potential UAF problem for phy_dev | expand |
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>
> 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> > . >
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; }
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(+)