| Message ID | 20241205103014.1625375-1-joe@pf.is.s.u-tokyo.ac.jp |
|---|---|
| State | New |
| Headers | show |
| Series | ata: sata_highbank: fix OF node reference leak in highbank_initialize_phys() | expand |
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 !
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
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
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;
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(+)