netdev - Re: [PATCH v2] net: phy: Fix return value when !CONFIG

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: <Z_vGfedEZGnkM6H0@shell.armlinux.org.uk>
Date: Sun, 13 Apr 2025 15:13:17 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: hhtracer@...il.com
Cc: andrew@...n.ch, hkallweit1@...il.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, huhai <huhai@...inos.cn>
Subject: Re: [PATCH v2] net: phy: Fix return value when !CONFIG_PHYLIB

On Sun, Apr 13, 2025 at 09:37:09PM +0800, hhtracer@...il.com wrote:
> Many call sites of get_phy_device() and fwnode_get_phy_node(), such as
> sfp_sm_probe_phy(), phylink_fwnode_phy_connect(), etc., rely on IS_ERR()
> to check for errors in the returned pointer.
> 
> Furthermore, the implementations of get_phy_device() and
> fwnode_get_phy_node() themselves use ERR_PTR() to return error codes.
> 
> Therefore, when CONFIG_PHYLIB is disabled, returning NULL is incorrect,
> as this would bypass IS_ERR() checks and may lead to NULL pointer
> dereference.
> 
> Returning ERR_PTR(-ENXIO) is the correct and consistent way to indicate
> that PHY support is not available, and it avoids such issues.

I wonder whether it's crossed one's mind that returning NULL may be
intentional to avoid erroring out at the callsites, and returning
an error may cause runtime failures.

You need to do way more investigation before posting a patch like this:

- Analyse each call site.
- Determine whether that call site can exist if PHYLIB is not built.
- Determine whether returning an error in that case instead of NULL
  may be detrimental, or at the very least list the call sites that
  would be affected by the change.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!