| Message ID | 20241216014055.324461-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] net: mdiobus: fix an OF node reference leak | expand |
On Mon, Dec 16, 2024 at 10:40:55AM +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. Just out of curiosity, have you improved this tool so it now reports the missing put you handled in version 3? I expect there is more code with the same error which a static analyser should be able to find when examining the abstract syntax tree. > +++ b/drivers/net/mdio/fwnode_mdio.c > @@ -41,6 +41,7 @@ static struct mii_timestamper * > fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) > { > struct of_phandle_args arg; > + struct mii_timestamper *mii_ts; > int err; The netdev subsystem wants variables declared longest first, shortest last, also known as reverse Christmas tree. As you work in different parts of the tree, you will find each subsystem has its own set of rules you will need to learn. > if (is_acpi_node(fwnode)) > @@ -53,10 +54,14 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) > else if (err) > return ERR_PTR(err); > > - if (arg.args_count != 1) > + if (arg.args_count != 1) { > + of_node_put(arg.np); > return ERR_PTR(-EINVAL); > + } > > - return register_mii_timestamper(arg.np, arg.args[0]); > + mii_ts = register_mii_timestamper(arg.np, arg.args[0]); > + of_node_put(arg.np); > + return mii_ts; > } Although this is correct, a more normal practice is to put all the cleanup at the end of the function and use a goto: if (arg.args_count != 1) { mii_ts = ERR_PTR(-EINVAL); goto put_node; } mii_ts = register_mii_timestamper(arg.np, arg.args[0]); put_node: of_node_put(arg.np); return mii_ts; } This tends to be more scalable, especially when more cleanup is required. Andrew
Thank you for your review. On 12/16/24 18:33, Andrew Lunn wrote: > On Mon, Dec 16, 2024 at 10:40:55AM +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. > > Just out of curiosity, have you improved this tool so it now reports > the missing put you handled in version 3? I expect there is more code > with the same error which a static analyser should be able to find > when examining the abstract syntax tree. Yes, and I am experimenting with other driver codes as well. > >> +++ b/drivers/net/mdio/fwnode_mdio.c >> @@ -41,6 +41,7 @@ static struct mii_timestamper * >> fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) >> { >> struct of_phandle_args arg; >> + struct mii_timestamper *mii_ts; >> int err; > > The netdev subsystem wants variables declared longest first, shortest > last, also known as reverse Christmas tree. As you work in different > parts of the tree, you will find each subsystem has its own set of > rules you will need to learn. TIL. Applied in the v4 patch. > >> if (is_acpi_node(fwnode)) >> @@ -53,10 +54,14 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) >> else if (err) >> return ERR_PTR(err); >> >> - if (arg.args_count != 1) >> + if (arg.args_count != 1) { >> + of_node_put(arg.np); >> return ERR_PTR(-EINVAL); >> + } >> >> - return register_mii_timestamper(arg.np, arg.args[0]); >> + mii_ts = register_mii_timestamper(arg.np, arg.args[0]); >> + of_node_put(arg.np); >> + return mii_ts; >> } > > Although this is correct, a more normal practice is to put all the > cleanup at the end of the function and use a goto: > > if (arg.args_count != 1) { > mii_ts = ERR_PTR(-EINVAL); > goto put_node; > } > > mii_ts = register_mii_timestamper(arg.np, arg.args[0]); > > put_node: > of_node_put(arg.np); > return mii_ts; > } > > This tends to be more scalable, especially when more cleanup is > required. Makes sense. Applied in the v4 patch as well. > > Andrew Best, Joe
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c index b156493d7084..b18a1934018e 100644 --- a/drivers/net/mdio/fwnode_mdio.c +++ b/drivers/net/mdio/fwnode_mdio.c @@ -41,6 +41,7 @@ static struct mii_timestamper * fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) { struct of_phandle_args arg; + struct mii_timestamper *mii_ts; int err; if (is_acpi_node(fwnode)) @@ -53,10 +54,14 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) else if (err) return ERR_PTR(err); - if (arg.args_count != 1) + if (arg.args_count != 1) { + of_node_put(arg.np); return ERR_PTR(-EINVAL); + } - return register_mii_timestamper(arg.np, arg.args[0]); + mii_ts = register_mii_timestamper(arg.np, arg.args[0]); + of_node_put(arg.np); + return mii_ts; } int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
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> --- Changes in v3: - Call of_node_put() when arg.args_count != 1 holds. Changes in v2: - Call of_node_put() after calling register_mii_timestamper() to avoid UAF. --- drivers/net/mdio/fwnode_mdio.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)