linux-kernel - Re: [PATCH] i2c: Fix reference leak in of_i2c_register

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <1dacfce7-c66d-44c6-9a0c-2dd00bc24ffc@wanadoo.fr>
Date: Tue, 8 Apr 2025 21:04:16 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Sunny <nueralspacetech@...il.com>
Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>,
 linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: Fix reference leak in of_i2c_register_devices

Hi,


first of all, you should not reply with HTML mail, because most mailing 
lists reject such messages.


Le 08/04/2025 à 16:01, Sunny a écrit :
> To clarify, while there is no early exit path in the 
> for_each_available_child_of_node() loop, the reference leak can still 
> occur because the macro increments the reference count for each node 
> it processes. If i2c_new_client_device() fails, the reference count 
> for that node must be explicitly decremented using of_node_put(node). 
> Without this, the reference count remains elevated, leading to a 
> reference leak.

I think you are wrong.

Yes, for_each_available_child_of_node() increments the reference count 
for each node it processes, but it also decrements it at the end of the 
iteration, except when there is an early exit (a break or a return).
See how the 'parent' and 'child' parameters of 
of_get_next_available_child() are used, especially whtat happen with the 
first call with child = NULL.

So should i2c_new_client_device() fail or succeed, 'node' is released 
and the reference count does NOT remain elevated as you state.


If you do not agree, please give more details of how you think it works 
and where the issue is, for exemple with unrolling the loop and noting 
what and when nodes are get and put.

You should then see that it is correct.


The of_node_put(node) you are looking for is there: 
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/of/base.c#L702

CJ

>
> sunny
>
> On Sun, 6 Apr 2025 at 23:36, Christophe JAILLET 
> <christophe.jaillet@...adoo.fr> wrote:
>
>     Le 06/04/2025 à 15:48, Sunny Patel a écrit :
>     > Fix a potential reference leak in of_i2c_register_devices where the
>     > reference to the node is not released if device registration fails.
>     > This ensures proper reference management and avoids memory leaks.
>
>     There is no early exit path in the for_each_available_child_of_node()
>     block, so of_node_put((node) is called for all the nodes that are
>     iterated.
>
>     Can you elaborate and explain how the reference leak can occur?
>
>     CJ
>
>     >
>     > Signed-off-by: Sunny Patel <nueralspacetech@...il.com>
>     > ---
>     >   drivers/i2c/i2c-core-of.c | 1 +
>     >   1 file changed, 1 insertion(+)
>     >
>     > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
>     > index 02feee6c9ba9..7c50905de8f1 100644
>     > --- a/drivers/i2c/i2c-core-of.c
>     > +++ b/drivers/i2c/i2c-core-of.c
>     > @@ -107,6 +107,7 @@ void of_i2c_register_devices(struct
>     i2c_adapter *adap)
>     >                                "Failed to create I2C device for
>     %pOF\n",
>     >                                node);
>     >                       of_node_clear_flag(node, OF_POPULATED);
>     > +                     of_node_put(node);
>     >               }
>     >       }
>     >
>