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

[Dan Carpenter <dan.carpenter@linaro.org>]

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@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;
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
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