On Mon, Dec 13, 2021 at 11:15:06AM +0100, Antoine Tenart wrote: > commit dde91ccfa25fd58f64c397d91b81a4b393100ffa upstream > > There is a short period between a net device starts to be unregistered > and when it is actually gone. In that time frame ethtool operations > could still be performed, which might end up in unwanted or undefined > behaviours[1]. > > Do not allow ethtool operations after a net device starts its > unregistration. This patch targets the netlink part as the ioctl one > isn't affected: the reference to the net device is taken and the > operation is executed within an rtnl lock section and the net device > won't be found after unregister. > > [1] For example adding Tx queues after unregister ends up in NULL > pointer exceptions and UaFs, such as: > > BUG: KASAN: use-after-free in kobject_get+0x14/0x90 > Read of size 1 at addr ffff88801961248c by task ethtool/755 > > CPU: 0 PID: 755 Comm: ethtool Not tainted 5.15.0-rc6+ #778 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-4.fc34 04/014 > Call Trace: > dump_stack_lvl+0x57/0x72 > print_address_description.constprop.0+0x1f/0x140 > kasan_report.cold+0x7f/0x11b > kobject_get+0x14/0x90 > kobject_add_internal+0x3d1/0x450 > kobject_init_and_add+0xba/0xf0 > netdev_queue_update_kobjects+0xcf/0x200 > netif_set_real_num_tx_queues+0xb4/0x310 > veth_set_channels+0x1c3/0x550 > ethnl_set_channels+0x524/0x610 > > [The patch differs from the upstream one as code was moved around by > commit 41107ac22fcf ("ethtool: move netif_device_present check from > ethnl_parse_header_dev_get to ethnl_ops_begin"). The check on the netdev > state is still done in ethnl_ops_begin as it must be done in an rtnl > section (the one which performs the op) to not race with > unregister_netdevice_many. > Also note the trace in [1] is not possible here as the channel ops for > veth were added later, but that was just one example.] > > Fixes: 041b1c5d4a53 ("ethtool: helper functions for netlink interface") > Suggested-by: Jakub Kicinski <kuba@xxxxxxxxxx> > Signed-off-by: Antoine Tenart <atenart@xxxxxxxxxx> > --- > > Hello, > > This patch is intended for the stable 5.10 tree. > > As reported by Greg, patch dde91ccfa25f ("ethtool: do not perform > operations on net devices being unregistered") did not apply correctly > on the 5.10 tree. The explanation of this and the approach taken here is > explained in the above commit log, between []. > > I removed the Link tag and Signed-off-by from Jakub from the original > patch as this one is slightly different in its implementation. > > Thanks, > Antoine > > net/ethtool/netlink.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h > index d8efec516d86..979dee6bb88c 100644 > --- a/net/ethtool/netlink.h > +++ b/net/ethtool/netlink.h > @@ -249,6 +249,9 @@ struct ethnl_reply_data { > > static inline int ethnl_ops_begin(struct net_device *dev) > { > + if (dev && dev->reg_state == NETREG_UNREGISTERING) > + return -ENODEV; > + > if (dev && dev->ethtool_ops->begin) > return dev->ethtool_ops->begin(dev); > else > -- > 2.33.1 > Now queued up, thanks. greg k-h