netdev - Re: [PATCH 18/18] net: dsa: microchip: Check return value of of_get_phy

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

Message-ID: <20240216010928.6gb6abjf5j356f76@skbuf>
Date: Fri, 16 Feb 2024 03:09:28 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Daniil Dulov <d.dulov@...ddin.ru>
Cc: Woojung Huh <woojung.huh@...rochip.com>, UNGLinuxDriver@...rochip.com,
	Andrew Lunn <andrew@...n.ch>,
	Vivien Didelot <vivien.didelot@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Helmut Grohne <helmut.grohne@...enta.de>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH 18/18] net: dsa: microchip: Check return value of
 of_get_phy_mode()

Hi Daniil,

On Sun, Feb 11, 2024 at 07:08:04AM -0800, Daniil Dulov wrote:
> of_get_phy_mode() may return an error, so add a check.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: edecfa98f602 ("net: dsa: microchip: look for phy-mode in port nodes")
> Signed-off-by: Daniil Dulov <d.dulov@...ddin.ru>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index e3c338624b95..faffce6ea1e1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -434,8 +434,10 @@ int ksz_switch_register(struct ksz_device *dev,
>  					continue;
>  				if (port_num >= dev->mib_port_cnt)
>  					return -EINVAL;
> -				of_get_phy_mode(port,
> +				ret = of_get_phy_mode(port,
>  						&dev->ports[port_num].interface);
> +				if (ret)
> +				    return ret;
>  			}
>  		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
>  							 "microchip,synclko-125");
> -- 
> 2.25.1
> 

Not all functions that may return an error, and that error is ignored,
must be fixed.

Here, if of_get_phy_mode() fails, dev->ports[port_num].interface remains
at its default value (0, or PHY_INTERFACE_MODE_NA). It is a valid value
to the rest of the driver at least in some cases (user ports connected
to internal PHYs). And since device tree is ABI, you are proposing to
break something that may have used to work, by making a previously well
treated path now cause an error.

Sorry, NACK even as a matter of principle, do not even bother to resend this
without a more careful analysis.