Re: [PATCH net] ethtool: check device is present when getting ioctl settings

[Jamie Bainbridge]

On Mon, 10 Feb 2025 at 10:51, Andrew Lunn <andrew@xxxxxxx> 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