netdev - Re: [PATCH net-next] ethtool: check for unsupported modes in EEE advertisement

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6ecc5c66-53b9-40c9-9fe4-b091afbe2f7f@gmail.com>
Date: Thu, 15 Feb 2024 21:27:33 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Paolo Abeni <pabeni@...hat.com>,
 Jakub Kicinski <kuba@...nel.org>, David Miller <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] ethtool: check for unsupported modes in EEE
 advertisement

On 15.02.2024 16:53, Russell King (Oracle) wrote:
> On Thu, Feb 15, 2024 at 02:05:54PM +0100, Heiner Kallweit wrote:
>> Let the core check whether userspace returned unsupported modes in the
>> EEE advertisement bitmap. This allows to remove these checks from
>> drivers.
> 
> Why is this a good thing to implement?
> 
Because it allows to remove all the duplicated checks from drivers.

> Concerns:
> 1) This is a change of behaviour for those drivers that do not
> implement this behaviour.
> 
Not of regular behavior. And at least for all drivers using phylib
it's no change.

> 2) This behaviour is different from ksettings_set() which silently
> trims the advertisement down to the modes that are supported
> 
It's the same check that we have in genphy_c45_ethtool_set_eee().
So it's in line with what we do in phylib.
But I would also be fine with silently trimming the advertisement.

> 3) This check is broken. Userspace is at liberty to pass in ~0 for
> the supported mask and the advertising mask which subverts this
> check.
> 
ethtool retrieves the supported mask with get_eee() from kernel.
And this (unmodified) value is passed with set_eee().
So at least with ethtool this scenario can't occur.

> So... I think overall, it's a NAK to this from me - I don't think
> it's something that anyone should implement. Restricting the
> advertisement to the modes that are supported (where the supported
> mask is pulled from the network driver and not userspace) would
> be acceptable, but is that actually necessary?
>