Messages in this thread | | | Subject | Re: [PATCH net v2] net: vlan: fix a UAF in vlan_dev_real_dev() | From | "Ziyang Xuan (William)" <> | Date | Fri, 26 Nov 2021 09:48:24 +0800 |
| |
> > 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. > .
Thank you for your efforts very much! I have sent the fix patch.
| |