[PATCH] ata: sata_highbank: fix OF node reference leak in highbank_initialize_phys()

[PATCH] ata: sata_highbank: fix OF node reference leak in highbank_initialize_phys()

[Joe Hattori]

The OF node reference obtained by of_parse_phandle_with_args() is not
released on early return. Add a of_node_put() call before returning.

Fixes: 8996b89d6bc9 ("ata: add platform driver for Calxeda AHCI controller")
Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
---
 drivers/ata/sata_highbank.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index b1b40e9551de..c8c817c51230 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -348,6 +348,7 @@ static int highbank_initialize_phys(struct device *dev, void __iomem *addr)
 			phy_nodes[phy] = phy_data.np;
 			cphy_base[phy] = of_iomap(phy_nodes[phy], 0);
 			if (cphy_base[phy] == NULL) {
+				of_node_put(phy_data.np);
 				return 0;
 			}
 			phy_count += 1;
-- 
2.34.1

Re: [PATCH] ata: sata_highbank: fix OF node reference leak in highbank_initialize_phys()

[Damien Le Moal]

On 12/5/24 19:30, Joe Hattori wrote:
> The OF node reference obtained by of_parse_phandle_with_args() is not
> released on early return. Add a of_node_put() call before returning.
> 
> Fixes: 8996b89d6bc9 ("ata: add platform driver for Calxeda AHCI controller")
> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>

Applied to for-6.13-fixes. Thanks !

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: sata_highbank: fix OF node reference leak in highbank_initialize_phys()

[Krzysztof Kozlowski]

On 09/12/2024 01:14, Damien Le Moal wrote:
> On 12/5/24 19:30, Joe Hattori wrote:
>> The OF node reference obtained by of_parse_phandle_with_args() is not
>> released on early return. Add a of_node_put() call before returning.
>>
>> Fixes: 8996b89d6bc9 ("ata: add platform driver for Calxeda AHCI controller")
>> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
> 
> Applied to for-6.13-fixes. Thanks !
Considering that:
1. Few other fixes reported by this static analysis were bogus and never
tested,
2. Missing of_node_put is entirely harmless, absolutely 0 effect, no
leak of anything, nothing to worry, no-op code currently,
3. But a mistakenly added incorrect of_node_put is a use-after-free bug,
4. This was in the kernel for long time, like 12 years (!!!),

then I really do not understand how it could be a 6.13-current-rc-fixes
material.

That's just wrong and possibly causing more harm. Really, please stop
sending trivial static analyzer fixes for 12 year old bug to current RC.

This was brought several times, last quote:

"I'm definitely not reverting a patch from almost a decade ago as a
regression.
If it took that long to find, it can't be that critical of a regression.
So yes, let's treat it as a regular bug."

Best regards,
Krzysztof

Re: [PATCH] ata: sata_highbank: fix OF node reference leak in highbank_initialize_phys()

[Niklas Cassel]

On Tue, Dec 17, 2024 at 12:15:35PM +0100, Krzysztof Kozlowski wrote:
> On 09/12/2024 01:14, Damien Le Moal wrote:
> > On 12/5/24 19:30, Joe Hattori wrote:
> >> The OF node reference obtained by of_parse_phandle_with_args() is not
> >> released on early return. Add a of_node_put() call before returning.
> >>
> >> Fixes: 8996b89d6bc9 ("ata: add platform driver for Calxeda AHCI controller")
> >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
> > 
> > Applied to for-6.13-fixes. Thanks !
> Considering that:
> 1. Few other fixes reported by this static analysis were bogus and never
> tested,
> 2. Missing of_node_put is entirely harmless, absolutely 0 effect, no
> leak of anything, nothing to worry, no-op code currently,
> 3. But a mistakenly added incorrect of_node_put is a use-after-free bug,
> 4. This was in the kernel for long time, like 12 years (!!!),
> 
> then I really do not understand how it could be a 6.13-current-rc-fixes
> material.
> 
> That's just wrong and possibly causing more harm. Really, please stop
> sending trivial static analyzer fixes for 12 year old bug to current RC.
> 
> This was brought several times, last quote:
> 
> "I'm definitely not reverting a patch from almost a decade ago as a
> regression.
> If it took that long to find, it can't be that critical of a regression.
> So yes, let's treat it as a regular bug."

For reference:
https://lore.kernel.org/lkml/CAHk-=wgFuoHpMk_Z_R3qMXVDgq0N1592+bABkyGjwwSL4zBtHA@mail.gmail.com/


Kind regards,
Niklas