Messages in this thread | | | From | Petr Machata <> | Subject | Re: [PATCH net v2] net: vlan: fix a UAF in vlan_dev_real_dev() | Date | Tue, 23 Nov 2021 13:35:35 +0100 |
| |
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.
| |