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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAAvyFNiTgZyVX79FztAB-4LGrq6ygKNXYYkOdLF0AY5TGF58ug@mail.gmail.com>
Date: Mon, 10 Feb 2025 11:17:28 +1000
From: Jamie Bainbridge <jamie.bainbridge@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: John J Coleman <jjcolemanx86@...il.com>, Jakub Kicinski <kuba@...nel.org>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Jiri Pirko <jiri@...nulli.us>, 
	Ben Hutchings <bhutchings@...arflare.com>, David Decotigny <decot@...glers.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] ethtool: check device is present when getting ioctl settings

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