Messages in this thread | | | From | Petr Machata <> | Subject | Re: [PATCH net v2] net: vlan: fix a UAF in vlan_dev_real_dev() | Date | Thu, 25 Nov 2021 12:33:11 +0100 |
| |
Petr Machata <petrm@nvidia.com> writes:
> Ziyang Xuan (William) <william.xuanziyang@huawei.com> writes: > >>> >>> Ziyang Xuan (William) <william.xuanziyang@huawei.com> writes: >>> >>>> I need some time to test my some ideas. And anyone has good ideas, please >>>> do not be stingy. >>> >>> Jakub Kicinski <kuba@kernel.org> writes: >>> >>>> I think we should move the dev_hold() to ndo_init(), otherwise it's >>>> hard to reason if destructor was invoked or not if >>>> register_netdevice() errors out. >>> >>> That makes sense to me. We always put real_dev in the destructor, so we >>> should always hold it in the constructor... >> >> Inject error before dev_hold(real_dev) in register_vlan_dev(), and execute >> the following testcase: >> >> ip link add dev dummy1 type dummy >> ip link add name dummy1.100 link dummy1 type vlan id 100 // failed for error injection >> ip link del dev dummy1 >> >> Make the problem repro. The problem is solved using the following fix >> according to the Jakub's suggestion: >> >> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c >> index a3a0a5e994f5..abaa5d96ded2 100644 >> --- a/net/8021q/vlan.c >> +++ b/net/8021q/vlan.c >> @@ -184,9 +184,6 @@ int register_vlan_dev(struct net_device *dev, struct netlink_ext_ack *extack) >> if (err) >> goto out_unregister_netdev; >> >> - /* Account for reference in struct vlan_dev_priv */ >> - dev_hold(real_dev); >> - >> vlan_stacked_transfer_operstate(real_dev, dev, vlan); >> linkwatch_fire_event(dev); /* _MUST_ call rfc2863_policy() */ >> >> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c >> index ab6dee28536d..a54535cbcf4c 100644 >> --- a/net/8021q/vlan_dev.c >> +++ b/net/8021q/vlan_dev.c >> @@ -615,6 +615,9 @@ static int vlan_dev_init(struct net_device *dev) >> if (!vlan->vlan_pcpu_stats) >> return -ENOMEM; >> >> + /* Get vlan's reference to real_dev */ >> + dev_hold(real_dev); >> >> >> If there is not any other idea and objection, I will send the fix patch later. >> >> Thank you! > > This fixes the issue in my repro, and does not seems to break anything. > We'll take it to full regression overnight, I'll report back tomorrow > about the result.
Sorry, was AFK yesterday. It does look good.
| |