[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <20221123160642.484567-1-alexandr.lobakin@intel.com>
Date: Wed, 23 Nov 2022 17:06:42 +0100
From: Alexander Lobakin <alexandr.lobakin@...el.com>
To: Daniil Tatianin <d-tatianin@...dex-team.ru>
Cc: Alexander Lobakin <alexandr.lobakin@...el.com>,
netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
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
From: Daniil Tatianin <d-tatianin@...dex-team.ru>
Date: Tue, 22 Nov 2022 10:21:43 +0300
> 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.
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE
> static analysis tool.
>
> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Daniil Tatianin <d-tatianin@...dex-team.ru>
> ---
> net/ethtool/ioctl.c | 31 ++++++++++++++++++-------------
> 1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 57e7238a4136..04f9ba98b038 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -2100,23 +2100,28 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
>
> stats.n_stats = n_stats;
>
> - if (n_stats) {
> - data = vzalloc(array_size(n_stats, sizeof(u64)));
> - if (!data)
> - return -ENOMEM;
> + if (!n_stats) {
> + data = NULL;
> + goto copy_back;
> + }
>
> - if (phydev && !ops->get_ethtool_phy_stats &&
> - phy_ops && phy_ops->get_stats) {
> - ret = phy_ops->get_stats(phydev, &stats, data);
> - if (ret < 0)
> - goto out;
> - } else {
> - ops->get_ethtool_phy_stats(dev, &stats, data);
> - }
> + data = vzalloc(array_size(n_stats, sizeof(u64)));
> + if (!data)
> + return -ENOMEM;
> +
> + if (ops->get_ethtool_phy_stats) {
> + ops->get_ethtool_phy_stats(dev, &stats, data);
I'd first check for the callback and only after allocate the array,
otherwise there's no optimization in here.
Also, I'd separate saving 1 level of indent from the functional
changes.
> + } else if (phydev && phy_ops && phy_ops->get_stats) {
> + ret = phy_ops->get_stats(phydev, &stats, data);
> + if (ret < 0)
> + goto out;
> } else {
> - data = NULL;
> + WARN_ON_ONCE(1);
> + n_stats = 0;
> + stats.n_stats = 0;
> }
>
> +copy_back:
> ret = -EFAULT;
> if (copy_to_user(useraddr, &stats, sizeof(stats)))
> goto out;
> --
> 2.25.1
Thanks,
Olek
Powered by blists - more mailing lists