Re: [PATCH] memory: tegra20-emc: fix an OF node reference bug in tegra_emc_find_node_by_ram_code()

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/12/2024 12:07, Joe Hattori wrote:
> 
> 
> On 12/17/24 18:31, Krzysztof Kozlowski wrote:
>> On 17/12/2024 10:14, Joe Hattori wrote:
>>> As of_find_node_by_name() release the reference of the given OF node,
>>
>> No, it does not.
> 
> I see in the document of the of_find_node_by_name() says that it calls 
> of_node_put(), or am I looking at the wrong code?

Hm, that's true that reference is put, but on the input node, not
returned one. I don't get to which node you are referring here thus
which node has double release or use-after-release.

Maybe it is all about incorrect dropping of this device's device node,
which should never happen in driver's probe path?


> /**
>   * of_find_node_by_name - Find a node by its "name" property
>   * @from:	The node to start searching from or NULL; the node
>   *		you pass will not be searched, only the next one
>   *		will. Typically, you pass what the previous call
>   *		returned. of_node_put() will be called on @from.
>   * @name:	The name string to match against
>   *
>   * Return: A node pointer with refcount incremented, use
>   * of_node_put() on it when done.
>   */
> 
> 
>>
>>> tegra_emc_find_node_by_ram_code() releases some OF nodes while still in
>>> use, resulting in possible UAFs. Given the DT structure, utilize the
>>> for_each_child_of_node macro and of_get_child_by_name() to avoid the bug.
>>>
>>> This bug was found by an experimental verification tool that I am
>>> developing.
>>>
>>> Fixes: 96e5da7c8424 ("memory: tegra: Introduce Tegra20 EMC driver")
>>> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
>>> ---
>>>   drivers/memory/tegra/tegra20-emc.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
>>> index 7193f848d17e..9b7d30a21a5b 100644
>>> --- a/drivers/memory/tegra/tegra20-emc.c
>>> +++ b/drivers/memory/tegra/tegra20-emc.c
>>> @@ -474,14 +474,15 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
>>>   
>>>   	ram_code = tegra_read_ram_code();
>>>   
>>> -	for (np = of_find_node_by_name(dev->of_node, "emc-tables"); np;
>>> -	     np = of_find_node_by_name(np, "emc-tables")) {
>>> +	for_each_child_of_node(dev->of_node, np) {
>>
>> I don't understand how this change is related to described problem.
> 
> As per the document, of_find_node_by_name() calls of_node_put(np), and 

In the first call no, it will of_node_put(from), not 'np'.
'from' != 'np'.


> the current code is calling of_node_put() before continuing the loop, so 
> the np can be released twice.

By the second release, you mean in the "if (cfg_mismatches)" path?
Otherwise there is no second release in the for loop.

> 
>>
>>> +		if (!of_node_name_eq(np, "emc-tables"))
>>> +			continue;
>>>   		err = of_property_read_u32(np, "nvidia,ram-code", &value);
>>>   		if (err || value != ram_code) {
>>>   			struct device_node *lpddr2_np;
>>>   			bool cfg_mismatches = false;
>>>   
>>> -			lpddr2_np = of_find_node_by_name(np, "lpddr2");
>>> +			lpddr2_np = of_get_child_by_name(np, "lpddr2");
>>
>> Why?
> 
> Given the Devicetree structure, I understand that calling 
> of_get_child_by_name() suffices here, which also does not release the 
> reference of np.

So you assume these have to be children. Is it tested with bindings?
With actual device?

> 
>>
>>>   			if (lpddr2_np) {
>>>   				const struct lpddr2_info *info;
>>>   
>>> @@ -518,7 +519,6 @@ tegra_emc_find_node_by_ram_code(struct tegra_emc *emc)
>>>   			}
>>>   
>>>   			if (cfg_mismatches) {
>>> -				of_node_put(np);
>>
>> If of_find_node_by_name() drops reference, why this was needed >
>>>   				continue;
>>


Best regards,
Krzysztof