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@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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
