| Message ID | 20241214081546.183159-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mdiobus: fix an OF node reference leak | expand |
On Sat, Dec 14, 2024 at 05:15:46PM +0900, Joe Hattori wrote: > fwnode_find_mii_timestamper() calls of_parse_phandle_with_fixed_args() > but does not decrement the refcount of the obtained OF node. Add an > of_node_put() call before returning from the function. > > This bug was detected by an experimental static analysis tool that I am > developing. > > Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > drivers/net/mdio/fwnode_mdio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c > index b156493d7084..83c8bd333117 100644 > --- a/drivers/net/mdio/fwnode_mdio.c > +++ b/drivers/net/mdio/fwnode_mdio.c > @@ -56,6 +56,7 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) > if (arg.args_count != 1) > return ERR_PTR(-EINVAL); > > + of_node_put(arg.np); > return register_mii_timestamper(arg.np, arg.args[0]); This looks wrong to me. If you do a put on an object, it can disappear, because it is not being used. You then pass it to register_mii_timestamper() and it gets to use something which no longer exists. Please think about what get/put are used for, and what that means for ordering. Maybe you can extend your tool to look for potential use after free bugs. Andrew --- pw-bot: cr
Hi Andrew, Thank you for your review. On 12/15/24 20:03, Andrew Lunn wrote: > On Sat, Dec 14, 2024 at 05:15:46PM +0900, Joe Hattori wrote: >> fwnode_find_mii_timestamper() calls of_parse_phandle_with_fixed_args() >> but does not decrement the refcount of the obtained OF node. Add an >> of_node_put() call before returning from the function. >> >> This bug was detected by an experimental static analysis tool that I am >> developing. >> >> Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()") >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> drivers/net/mdio/fwnode_mdio.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c >> index b156493d7084..83c8bd333117 100644 >> --- a/drivers/net/mdio/fwnode_mdio.c >> +++ b/drivers/net/mdio/fwnode_mdio.c >> @@ -56,6 +56,7 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) >> if (arg.args_count != 1) >> return ERR_PTR(-EINVAL); >> >> + of_node_put(arg.np); >> return register_mii_timestamper(arg.np, arg.args[0]); > > This looks wrong to me. If you do a put on an object, it can > disappear, because it is not being used. You then pass it to > register_mii_timestamper() and it gets to use something which no > longer exists. Totally. Should have realized that. It is fixed in the v2 patch as I moved the of_node_put() call to the very end of the function. > > Please think about what get/put are used for, and what that means for > ordering. > > Maybe you can extend your tool to look for potential use after free > bugs. > > Andrew > > --- > pw-bot: cr Best, Joe
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c index b156493d7084..83c8bd333117 100644 --- a/drivers/net/mdio/fwnode_mdio.c +++ b/drivers/net/mdio/fwnode_mdio.c @@ -56,6 +56,7 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) if (arg.args_count != 1) return ERR_PTR(-EINVAL); + of_node_put(arg.np); return register_mii_timestamper(arg.np, arg.args[0]); }
fwnode_find_mii_timestamper() calls of_parse_phandle_with_fixed_args() but does not decrement the refcount of the obtained OF node. Add an of_node_put() call before returning from the function. This bug was detected by an experimental static analysis tool that I am developing. Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- drivers/net/mdio/fwnode_mdio.c | 1 + 1 file changed, 1 insertion(+)