* [PATCH net] ethtool: do not perform operations on net devices being unregistered
@ 2021-12-03 10:13 Antoine Tenart
2021-12-06 9:46 ` Julian Wiedmann
2021-12-07 1:00 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 6+ messages in thread
From: Antoine Tenart @ 2021-12-03 10:13 UTC (permalink / raw)
To: davem, kuba; +Cc: Antoine Tenart, alexander.duyck, mkubecek, netdev
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
Fixes: 041b1c5d4a53 ("ethtool: helper functions for netlink interface")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
Following the discussions in those threads:
- https://lore.kernel.org/all/20211129154520.295823-1-atenart@kernel.org/T/
- https://lore.kernel.org/all/20211122162007.303623-1-atenart@kernel.org/T/
net/ethtool/netlink.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 38b44c0291b1..96f4180aabd2 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -40,7 +40,8 @@ int ethnl_ops_begin(struct net_device *dev)
if (dev->dev.parent)
pm_runtime_get_sync(dev->dev.parent);
- if (!netif_device_present(dev)) {
+ if (!netif_device_present(dev) ||
+ dev->reg_state == NETREG_UNREGISTERING) {
ret = -ENODEV;
goto err;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: do not perform operations on net devices being unregistered
2021-12-03 10:13 [PATCH net] ethtool: do not perform operations on net devices being unregistered Antoine Tenart
@ 2021-12-06 9:46 ` Julian Wiedmann
2021-12-06 15:15 ` Jakub Kicinski
2021-12-07 1:00 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 6+ messages in thread
From: Julian Wiedmann @ 2021-12-06 9:46 UTC (permalink / raw)
To: Antoine Tenart, davem, kuba; +Cc: alexander.duyck, mkubecek, netdev
On 03.12.21 12:13, Antoine Tenart wrote:
> 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
>
> Fixes: 041b1c5d4a53 ("ethtool: helper functions for netlink interface")
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Antoine Tenart <atenart@kernel.org>
> ---
>
> Following the discussions in those threads:
> - https://lore.kernel.org/all/20211129154520.295823-1-atenart@kernel.org/T/
> - https://lore.kernel.org/all/20211122162007.303623-1-atenart@kernel.org/T/
>
> net/ethtool/netlink.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 38b44c0291b1..96f4180aabd2 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -40,7 +40,8 @@ int ethnl_ops_begin(struct net_device *dev)
> if (dev->dev.parent)
> pm_runtime_get_sync(dev->dev.parent);
>
> - if (!netif_device_present(dev)) {
> + if (!netif_device_present(dev) ||
> + dev->reg_state == NETREG_UNREGISTERING) {
> ret = -ENODEV;
> goto err;
> }
>
Wondering if other places would also benefit from a netif_device_detach()
in the unregistration sequence ...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: do not perform operations on net devices being unregistered
2021-12-06 9:46 ` Julian Wiedmann
@ 2021-12-06 15:15 ` Jakub Kicinski
2021-12-07 8:05 ` Antoine Tenart
2021-12-07 9:05 ` Julian Wiedmann
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Kicinski @ 2021-12-06 15:15 UTC (permalink / raw)
To: Julian Wiedmann; +Cc: Antoine Tenart, davem, alexander.duyck, mkubecek, netdev
On Mon, 6 Dec 2021 11:46:35 +0200 Julian Wiedmann wrote:
> On 03.12.21 12:13, Antoine Tenart wrote:
> > 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.
> > [...]
> > +++ b/net/ethtool/netlink.c
> > @@ -40,7 +40,8 @@ int ethnl_ops_begin(struct net_device *dev)
> > if (dev->dev.parent)
> > pm_runtime_get_sync(dev->dev.parent);
> >
> > - if (!netif_device_present(dev)) {
> > + if (!netif_device_present(dev) ||
> > + dev->reg_state == NETREG_UNREGISTERING) {
> > ret = -ENODEV;
> > goto err;
> > }
> >
>
> Wondering if other places would also benefit from a netif_device_detach()
> in the unregistration sequence ...
Sounds like a good idea but maybe as a follow up to net-next?
The likelihood of that breaking things is low, but non-zero.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: do not perform operations on net devices being unregistered
2021-12-03 10:13 [PATCH net] ethtool: do not perform operations on net devices being unregistered Antoine Tenart
2021-12-06 9:46 ` Julian Wiedmann
@ 2021-12-07 1:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-12-07 1:00 UTC (permalink / raw)
To: Antoine Tenart; +Cc: davem, kuba, alexander.duyck, mkubecek, netdev
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Fri, 3 Dec 2021 11:13:18 +0100 you wrote:
> 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.
>
> [...]
Here is the summary with links:
- [net] ethtool: do not perform operations on net devices being unregistered
https://git.kernel.org/netdev/net/c/dde91ccfa25f
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: do not perform operations on net devices being unregistered
2021-12-06 15:15 ` Jakub Kicinski
@ 2021-12-07 8:05 ` Antoine Tenart
2021-12-07 9:05 ` Julian Wiedmann
1 sibling, 0 replies; 6+ messages in thread
From: Antoine Tenart @ 2021-12-07 8:05 UTC (permalink / raw)
To: Jakub Kicinski, Julian Wiedmann; +Cc: davem, alexander.duyck, mkubecek, netdev
Quoting Jakub Kicinski (2021-12-06 16:15:20)
> On Mon, 6 Dec 2021 11:46:35 +0200 Julian Wiedmann wrote:
> >
> > Wondering if other places would also benefit from a netif_device_detach()
> > in the unregistration sequence ...
>
> Sounds like a good idea but maybe as a follow up to net-next?
> The likelihood of that breaking things is low, but non-zero.
Might be good to have a look at this yes. I'm wondering, there are
multiple mechanisms to avoid using a net device after unregistration,
including netif_device_detach and unlinking it from the device chain
(unlist_netdevice). Currently it's possible to have a device unlinked
but not detached. Haven't looked at it, but there might be something to
do here.
(One the other way, detaching a device without unlinking it is valid for
PM).
Antoine
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ethtool: do not perform operations on net devices being unregistered
2021-12-06 15:15 ` Jakub Kicinski
2021-12-07 8:05 ` Antoine Tenart
@ 2021-12-07 9:05 ` Julian Wiedmann
1 sibling, 0 replies; 6+ messages in thread
From: Julian Wiedmann @ 2021-12-07 9:05 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Antoine Tenart, davem, alexander.duyck, mkubecek, netdev
On 06.12.21 17:15, Jakub Kicinski wrote:
> On Mon, 6 Dec 2021 11:46:35 +0200 Julian Wiedmann wrote:
>> On 03.12.21 12:13, Antoine Tenart wrote:
>>> 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.
>>> [...]
>>> +++ b/net/ethtool/netlink.c
>>> @@ -40,7 +40,8 @@ int ethnl_ops_begin(struct net_device *dev)
>>> if (dev->dev.parent)
>>> pm_runtime_get_sync(dev->dev.parent);
>>>
>>> - if (!netif_device_present(dev)) {
>>> + if (!netif_device_present(dev) ||
>>> + dev->reg_state == NETREG_UNREGISTERING) {
>>> ret = -ENODEV;
>>> goto err;
>>> }
>>>
>>
>> Wondering if other places would also benefit from a netif_device_detach()
>> in the unregistration sequence ...
>
> Sounds like a good idea but maybe as a follow up to net-next?
> The likelihood of that breaking things is low, but non-zero.
>
Oh absolutely, only via net-next.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-07 9:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-03 10:13 [PATCH net] ethtool: do not perform operations on net devices being unregistered Antoine Tenart
2021-12-06 9:46 ` Julian Wiedmann
2021-12-06 15:15 ` Jakub Kicinski
2021-12-07 8:05 ` Antoine Tenart
2021-12-07 9:05 ` Julian Wiedmann
2021-12-07 1:00 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.