netdev - Re: [PATCH v3] net: mdiobus: 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: <c7b55b3d-c7a9-4d42-a6e4-64148816a80d@lunn.ch>
Date: Mon, 16 Dec 2024 10:33:50 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Joe Hattori <joe@...is.s.u-tokyo.ac.jp>
Cc: hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH v3] net: mdiobus: fix an OF node reference leak

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