netdev - Re: [PATCH] net: ethernet: bgmac-platform: fix an OF node reference leak

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: <b8604925-d3fb-4994-893c-d34e6185e950@lunn.ch>
Date: Fri, 13 Dec 2024 13:04:42 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Simon Horman <horms@...nel.org>
Cc: Joe Hattori <joe@...is.s.u-tokyo.ac.jp>, rafal@...ecki.pl,
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com,
	bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org
Subject: Re: [PATCH] net: ethernet: bgmac-platform: fix an OF node reference
 leak

> Hi Joe,
> 
> I agree this is a problem and that it was introduced by the
> cited commit. But I wonder if we can consider a different approach.
> 
> I would suggest that rather than using __free the node is explicitly
> released. Something like this (untested):
> 
> 	struct device_node *phy_node;
> 
> 	...
> 
> 	phy_node = of_parse_phandle(np, "phy-handle", 0);
> 	if (phy_node) {
> 		of_node_put(phy_node);
> 		bgmac->phy_connect = platform_phy_connect;
> 	} ...
> 
> That is, assuming that it is safe to release phy_node so early.
> If not, some adjustment should be made to when of_node_put()
> is called.
> 
> This is for several reasons;
> 
> 1. I could be wrong, but I believe your patch kfree's phy_node,
>    but my understanding is that correct operation is to call
>    of_node_put().

Hi Simon

I _think_ that is wrong. More of the magic which i don't really
like. The cleanup subsystem has to be taught all the types, and what
operation to perform for each type. Despite the name __free(), i think
it does actually call of_node_put(). The magic would be more readable
if it was actually __put(), not __free().

> 2. More importantly, there is a preference in Newkorking code
>    not to use __free and similar constructs.
> 
>      "Low level cleanup constructs (such as __free()) can be used when
>       building APIs and helpers, especially scoped iterators. However,
>       direct use of __free() within networking core and drivers is
>       discouraged. Similar guidance applies to declaring variables
>       mid-function.

And this is a good example of why.

	Andrew