| Message ID | 20250210003200.368428-1-jjcolemanx86@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ethtool: check device is present when getting ioctl settings | expand |
On Sun, Feb 09, 2025 at 05:31:56PM -0700, John J Coleman wrote: > An ioctl caller of SIOCETHTOOL ETHTOOL_GSET can provoke the legacy > ethtool codepath on a non-present device, leading to kernel panic: > > [exception RIP: qed_get_current_link+0x11] > #8 [ffffa2021d70f948] qede_get_link_ksettings at ffffffffc07bfa9a [qede] > #9 [ffffa2021d70f9d0] __rh_call_get_link_ksettings at ffffffff9bad2723 > #10 [ffffa2021d70fa30] ethtool_get_settings at ffffffff9bad29d0 > #11 [ffffa2021d70fb18] __dev_ethtool at ffffffff9bad442b > #12 [ffffa2021d70fc28] dev_ethtool at ffffffff9bad6db8 > #13 [ffffa2021d70fc60] dev_ioctl at ffffffff9ba7a55c > #14 [ffffa2021d70fc98] sock_do_ioctl at ffffffff9ba22a44 > #15 [ffffa2021d70fd08] sock_ioctl at ffffffff9ba22d1c > #16 [ffffa2021d70fd78] do_vfs_ioctl at ffffffff9b584cf4 > > Device is not present with no state bits set: > > crash> net_device.state ffff8fff95240000 > state = 0x0, > > Existing patch commit a699781c79ec ("ethtool: check device is present > when getting link settings") fixes this in the modern sysfs reader's > ksettings path. > > Fix this in the legacy ioctl path by checking for device presence as > well. What is not clear to my is why ethtool_get_settings() is special. Why does ethtool_set_settings() not suffer from the same problem, or any of the other ioctls? Andrew
On Mon, 10 Feb 2025 at 10:51, Andrew Lunn <andrew@lunn.ch> wrote: > > On Sun, Feb 09, 2025 at 05:31:56PM -0700, John J Coleman wrote: > > An ioctl caller of SIOCETHTOOL ETHTOOL_GSET can provoke the legacy > > ethtool codepath on a non-present device, leading to kernel panic: > > > > [exception RIP: qed_get_current_link+0x11] > > #8 [ffffa2021d70f948] qede_get_link_ksettings at ffffffffc07bfa9a [qede] > > #9 [ffffa2021d70f9d0] __rh_call_get_link_ksettings at ffffffff9bad2723 > > #10 [ffffa2021d70fa30] ethtool_get_settings at ffffffff9bad29d0 > > #11 [ffffa2021d70fb18] __dev_ethtool at ffffffff9bad442b > > #12 [ffffa2021d70fc28] dev_ethtool at ffffffff9bad6db8 > > #13 [ffffa2021d70fc60] dev_ioctl at ffffffff9ba7a55c > > #14 [ffffa2021d70fc98] sock_do_ioctl at ffffffff9ba22a44 > > #15 [ffffa2021d70fd08] sock_ioctl at ffffffff9ba22d1c > > #16 [ffffa2021d70fd78] do_vfs_ioctl at ffffffff9b584cf4 > > > > Device is not present with no state bits set: > > > > crash> net_device.state ffff8fff95240000 > > state = 0x0, > > > > Existing patch commit a699781c79ec ("ethtool: check device is present > > when getting link settings") fixes this in the modern sysfs reader's > > ksettings path. > > > > Fix this in the legacy ioctl path by checking for device presence as > > well. > > What is not clear to my is why ethtool_get_settings() is special. Why > does ethtool_set_settings() not suffer from the same problem, or any > of the other ioctls? ethtool_set_settings() would suffer the same problem. Last time I did this (with what became a699781c79ec) I was discouraged from fixing additional theoretical problems which weren't the actual problem I faced. We did not review other ioctls. Looking now, I see commit f32a213765739 ("ethtool: runtime-resume netdev parent before ethtool ioctl ops") would have protected against this as it adds the netif_device_present() check one function back in dev_ethtool(). We do not yet have that commit in our kernel. It seems we can forget this. Many thanks for the review Andrew. Jamie
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 7609ce2b2c5e2ead90aceab08b6610955914340b..1d7c72d7bb9a0fcbb8d47556ec3173440db32447 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -659,6 +659,9 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr) int err; ASSERT_RTNL(); + if (!netif_device_present(dev)) + return -ENODEV; + if (!dev->ethtool_ops->get_link_ksettings) return -EOPNOTSUPP;
An ioctl caller of SIOCETHTOOL ETHTOOL_GSET can provoke the legacy ethtool codepath on a non-present device, leading to kernel panic: [exception RIP: qed_get_current_link+0x11] #8 [ffffa2021d70f948] qede_get_link_ksettings at ffffffffc07bfa9a [qede] #9 [ffffa2021d70f9d0] __rh_call_get_link_ksettings at ffffffff9bad2723 #10 [ffffa2021d70fa30] ethtool_get_settings at ffffffff9bad29d0 #11 [ffffa2021d70fb18] __dev_ethtool at ffffffff9bad442b #12 [ffffa2021d70fc28] dev_ethtool at ffffffff9bad6db8 #13 [ffffa2021d70fc60] dev_ioctl at ffffffff9ba7a55c #14 [ffffa2021d70fc98] sock_do_ioctl at ffffffff9ba22a44 #15 [ffffa2021d70fd08] sock_ioctl at ffffffff9ba22d1c #16 [ffffa2021d70fd78] do_vfs_ioctl at ffffffff9b584cf4 Device is not present with no state bits set: crash> net_device.state ffff8fff95240000 state = 0x0, Existing patch commit a699781c79ec ("ethtool: check device is present when getting link settings") fixes this in the modern sysfs reader's ksettings path. Fix this in the legacy ioctl path by checking for device presence as well. Fixes: 4bc71cb983fd2 ("net: consolidate and fix ethtool_ops->get_settings calling") Fixes: 3f1ac7a700d03 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS API") Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Tested-by: John J Coleman <jjcolemanx86@gmail.com> Co-developed-by: Jamie Bainbridge <jamie.bainbridge@gmail.com> Signed-off-by: Jamie Bainbridge <jamie.bainbridge@gmail.com> Signed-off-by: John J Coleman <jjcolemanx86@gmail.com> --- net/ethtool/ioctl.c | 3 +++ 1 file changed, 3 insertions(+)