netdev - Re: [PATCH v2] net/ethtool/ioctl: ensure that we have phy ops before using them

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: <Y30I0d6Dd+s9Ak0W@lunn.ch>
Date:   Tue, 22 Nov 2022 18:37:21 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Daniil Tatianin <d-tatianin@...dex-team.ru>
Cc:     netdev@...r.kernel.org, Michal Kubecek <mkubecek@...e.cz>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH v2] net/ethtool/ioctl: ensure that we have phy ops before
 using them

On Tue, Nov 22, 2022 at 10:21:43AM +0300, Daniil Tatianin wrote:
> ops->get_ethtool_phy_stats was getting called in an else branch
> of ethtool_get_phy_stats() unconditionally without making sure
> it was actually present.
> 
> Refactor the checks to avoid unnecessary nesting and make them more
> readable. Add an extra WARN_ON_ONCE(1) to emit a warning when a driver
> declares that it has phy stats without a way to retrieve them.

So i have two different suggestions here, take your pick and even
merge them together.

I wonder if we can simply this some more. If there are 0 stats we
already issue a WARN_ON_ONCE():

https://elixir.bootlin.com/linux/v6.1-rc6/source/net/ethtool/ioctl.c#L2096

We will then copy back to user space the ethtool_stats and zero
statistics and return 0. If that useful? Would it make more sense to
just return -EOPNOTSUPP after the WARN_ON_ONCE()?

That would be patch 1/X.

Patch 2/X would then remove the if (n_stats) code, but otherwise make
no changes. That keeps the patch simple to review.

Patch 3/X would then add the additional verification of
ops->get_ethtool_phy_stats(). But do it at the top of the function,
along with all the other verification, and return -EOPNOTSUPP.

Alternatively, given the complexity of the checking and the function
as a whole, i'm wondering if it make sense to actually pull this
function apart. Add a ethtool_get_phy_stats_phydev() and
ethtool_get_phy_stats_ethtool(), and have ethtool_get_phy_stats() do
the copy_from_user(), call one of the two helpers, and if they don't
return an error do the two copy_to_user().

       Andrew