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

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

[Daniil Tatianin]

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@kernel.org>
Signed-off-by: Daniil Tatianin <d-tatianin@yandex-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);
+	} 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

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

[Andrew Lunn]

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

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

[Alexander Lobakin]

From: Daniil Tatianin <d-tatianin@yandex-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@kernel.org>
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-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

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

[Jakub Kicinski]

On Mon, 21 Nov 2022 17:05:56 +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.
> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE
> static analysis tool.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>

Didn't make it to the list again :S Maybe try stripping the To/CC to
just netdev@, Andrew Lunn and Michal Kubecek?