On Tue, Dec 17, 2024 at 10:31:23AM +0100, 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.
>
Yeah, it does.
drivers/of/base.c
927 /**
928 * of_find_node_by_name - Find a node by its "name" property
929 * @from: The node to start searching from or NULL; the node
930 * you pass will not be searched, only the next one
931 * will. Typically, you pass what the previous call
932 * returned. of_node_put() will be called on @from.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
933 * @name: The name string to match against
934 *
935 * Return: A node pointer with refcount incremented, use
936 * of_node_put() on it when done.
937 */
938 struct device_node *of_find_node_by_name(struct device_node *from,
939 const char *name)
940 {
941 struct device_node *np;
942 unsigned long flags;
943
944 raw_spin_lock_irqsave(&devtree_lock, flags);
945 for_each_of_allnodes_from(from, np)
946 if (of_node_name_eq(np, name) && of_node_get(np))
947 break;
948 of_node_put(from);
^^^^^
949 raw_spin_unlock_irqrestore(&devtree_lock, flags);
950 return np;
951 }
> > 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;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This original code is wrong.
> > - 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.
>
> > + 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?
This drops the reference on "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?
The continue statement also drops the reference. So this code as an
accidental of_node_put(dev->of_node) and two accidental extra calls to
of_node_put(np).
I can't say if the fix is correct, but the bug is real.
regards,
dan carpenter
