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?
/**
* 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
the current code is calling of_node_put() before continuing the loop, so
the np can be released twice.
+ 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.
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
Best,
Joe
