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: <3604a59e-4287-428f-b4ba-8528d2ffdbe9@gmail.com>
Date: Thu, 15 Feb 2024 22:08:40 +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 21:27, Heiner Kallweit wrote:
> 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.
> 
In addition: In the netlink case the supported mask isn't even
transferred to userspace for the set_eee operation.

>> 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?
>>
>