[RFC PATCH 0/4] Dynamic OF and use after free related fixes

[RFC PATCH 0/4] Dynamic OF and use after free related fixes

[Nuno Sá]

This series is a collection of fixes with things I encountered when
handling a system with device tree overlays "fun". This effectively means
removing and adding devices which led to me the found issues.

patch 1 is fairly straight and indeed an issue related with of_nodes not
being put.

For patches 2 and 3, they are only triggered if someone first removes the
provider which is a dumb thing to do. However, I guess we should be
prepared for that and the framework even has some efforts to deal with
these cases (eg: clk_nodrv_ops). That said, patch 2 is more or less
straight (even though catching the guilty line wasn't), we add something
to a list during resgister, we need to remove it on the unregister path.
Patch 3 is a different beast... though I think krefs are the proper solution
here, I'm not sure I covered all the corner cases with all the reparenting
that might happen (mainly reason why this is RFC). Nullyfing
clk_core->parent during unregister might suffice but I'm more in favor of
proper refcounting (for now at least). Patch 3 also does not have a fixes
tag because I genuily don't know when this became a problem. Maybe right
from the beginning?

Patch 4 is just a minor improvement and not really a fix...

Nuno Sá (4):
  clk: clk-conf: properly release of nodes
  clk: fix clk not being unlinked from consumers list
  clk: refcount the active parent clk_core
  clk: use clk_core_unlink_consumer() helper

 drivers/clk/clk-conf.c | 18 +++++++--
 drivers/clk/clk.c      | 83 +++++++++++++++++++++++++++---------------
 2 files changed, 67 insertions(+), 34 deletions(-)

-- 
2.35.1

[RFC PATCH 1/4] clk: clk-conf: properly release of nodes

[Nuno Sá]

We need to call 'of_node_put()' when we are done with the node or on
error paths. Otherwise this can leak memory in DYNAMIC_OF setups.

Fixes: 86be408bfbd8 ("clk: Support for clock parents and rates assigned from device tree")
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
(cherry picked from commit 69eb47a26e7f728a5c052687380993cd9a0dd643)
---
 drivers/clk/clk-conf.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 2ef819606c41..d6065cdf1540 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -33,9 +33,12 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			else
 				return rc;
 		}
-		if (clkspec.np == node && !clk_supplier)
+		if (clkspec.np == node && !clk_supplier) {
+			of_node_put(clkspec.np);
 			return 0;
+		}
 		pclk = of_clk_get_from_provider(&clkspec);
+		of_node_put(clkspec.np);
 		if (IS_ERR(pclk)) {
 			if (PTR_ERR(pclk) != -EPROBE_DEFER)
 				pr_warn("clk: couldn't get parent clock %d for %pOF\n",
@@ -49,7 +52,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			goto err;
 		if (clkspec.np == node && !clk_supplier) {
 			rc = 0;
-			goto err;
+			goto err_of_put;
 		}
 		clk = of_clk_get_from_provider(&clkspec);
 		if (IS_ERR(clk)) {
@@ -57,7 +60,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 				pr_warn("clk: couldn't get assigned clock %d for %pOF\n",
 					index, node);
 			rc = PTR_ERR(clk);
-			goto err;
+			goto err_of_put;
 		}
 
 		rc = clk_set_parent(clk, pclk);
@@ -66,8 +69,11 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			       __clk_get_name(clk), __clk_get_name(pclk), rc);
 		clk_put(clk);
 		clk_put(pclk);
+		of_node_put(clkspec.np);
 	}
 	return 0;
+err_of_put:
+	of_node_put(clkspec.np);
 err:
 	clk_put(pclk);
 	return rc;
@@ -93,14 +99,17 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 				else
 					return rc;
 			}
-			if (clkspec.np == node && !clk_supplier)
+			if (clkspec.np == node && !clk_supplier) {
+				of_node_put(clkspec.np);
 				return 0;
+			}
 
 			clk = of_clk_get_from_provider(&clkspec);
 			if (IS_ERR(clk)) {
 				if (PTR_ERR(clk) != -EPROBE_DEFER)
 					pr_warn("clk: couldn't get clock %d for %pOF\n",
 						index, node);
+				of_node_put(clkspec.np);
 				return PTR_ERR(clk);
 			}
 
@@ -110,6 +119,7 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 				       __clk_get_name(clk), rate, rc,
 				       clk_get_rate(clk));
 			clk_put(clk);
+			of_node_put(clkspec.np);
 		}
 		index++;
 	}
-- 
2.35.1

Re: [RFC PATCH 1/4] clk: clk-conf: properly release of nodes

[Stephen Boyd]

Quoting Nuno Sá (2022-04-07 06:30:33)
> We need to call 'of_node_put()' when we are done with the node or on
> error paths. Otherwise this can leak memory in DYNAMIC_OF setups.
> 
> Fixes: 86be408bfbd8 ("clk: Support for clock parents and rates assigned from device tree")
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> (cherry picked from commit 69eb47a26e7f728a5c052687380993cd9a0dd643)

This line should be removed.

> ---
>  drivers/clk/clk-conf.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> index 2ef819606c41..d6065cdf1540 100644
> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -33,9 +33,12 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
>                         else
>                                 return rc;
>                 }
> -               if (clkspec.np == node && !clk_supplier)
> +               if (clkspec.np == node && !clk_supplier) {
> +                       of_node_put(clkspec.np);
>                         return 0;
> +               }
>                 pclk = of_clk_get_from_provider(&clkspec);
> +               of_node_put(clkspec.np);
>                 if (IS_ERR(pclk)) {
>                         if (PTR_ERR(pclk) != -EPROBE_DEFER)
>                                 pr_warn("clk: couldn't get parent clock %d for %pOF\n",

I suspect it would be easier to follow and fix if the code was
reorganized to have most of the contents inside this function as a
sub-routine that is called for each index. Then all the node putting and
clk putting can be in one place in that other function and this is a
simple loop around that stops on the first error.

> @@ -49,7 +52,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
>                         goto err;
>                 if (clkspec.np == node && !clk_supplier) {
>                         rc = 0;
> -                       goto err;
> +                       goto err_of_put;
>                 }
>                 clk = of_clk_get_from_provider(&clkspec);
>                 if (IS_ERR(clk)) {

RE: [RFC PATCH 1/4] clk: clk-conf: properly release of nodes

[Sa, Nuno]

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Thursday, April 21, 2022 9:58 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; linux-clk@vger.kernel.org
> Cc: Michael Turquette <mturquette@baylibre.com>
> Subject: Re: [RFC PATCH 1/4] clk: clk-conf: properly release of nodes
> 
> [External]
> 
> Quoting Nuno Sá (2022-04-07 06:30:33)
> > We need to call 'of_node_put()' when we are done with the node or
> on
> > error paths. Otherwise this can leak memory in DYNAMIC_OF setups.
> >
> > Fixes: 86be408bfbd8 ("clk: Support for clock parents and rates
> assigned from device tree")
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > (cherry picked from commit
> 69eb47a26e7f728a5c052687380993cd9a0dd643)
> 
> This line should be removed.
> 

Yes, I know :). I Was just waiting for the review. Somehow I messed
up when preparing the patches to submit...

- Nuno Sá

RE: [RFC PATCH 1/4] clk: clk-conf: properly release of nodes

[Sa, Nuno]

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Thursday, April 21, 2022 9:58 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; linux-clk@vger.kernel.org
> Cc: Michael Turquette <mturquette@baylibre.com>
> Subject: Re: [RFC PATCH 1/4] clk: clk-conf: properly release of nodes
> 
> [External]
> 
> Quoting Nuno Sá (2022-04-07 06:30:33)
> > We need to call 'of_node_put()' when we are done with the node or
> on
> > error paths. Otherwise this can leak memory in DYNAMIC_OF setups.
> >
> > Fixes: 86be408bfbd8 ("clk: Support for clock parents and rates
> assigned from device tree")
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > (cherry picked from commit
> 69eb47a26e7f728a5c052687380993cd9a0dd643)
> 
> This line should be removed.
> 
> > ---
> >  drivers/clk/clk-conf.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> > index 2ef819606c41..d6065cdf1540 100644
> > --- a/drivers/clk/clk-conf.c
> > +++ b/drivers/clk/clk-conf.c
> > @@ -33,9 +33,12 @@ static int __set_clk_parents(struct
> device_node *node, bool clk_supplier)
> >                         else
> >                                 return rc;
> >                 }
> > -               if (clkspec.np == node && !clk_supplier)
> > +               if (clkspec.np == node && !clk_supplier) {
> > +                       of_node_put(clkspec.np);
> >                         return 0;
> > +               }
> >                 pclk = of_clk_get_from_provider(&clkspec);
> > +               of_node_put(clkspec.np);
> >                 if (IS_ERR(pclk)) {
> >                         if (PTR_ERR(pclk) != -EPROBE_DEFER)
> >                                 pr_warn("clk: couldn't get parent clock %d for
> %pOF\n",
> 
> I suspect it would be easier to follow and fix if the code was
> reorganized to have most of the contents inside this function as a
> sub-routine that is called for each index. Then all the node putting and
> clk putting can be in one place in that other function and this is a
> simple loop around that stops on the first error.
> 

Ups, forgot to answer this one. Yeah, I did not wanted to change much but
I agree that the code could be a bit refactored and I can do that in v2.

- Nuno Sá

[RFC PATCH 2/4] clk: fix clk not being unlinked from consumers list

[Nuno Sá]

When a clk_hw is resgistered we add a struct clk handle to it's
consumers list. Hence, we need to remove it when unregistering the
clk_hw. This could actually lead to a use after free if a provider get's
removed before a consumer. When removing the consumer, __clk_put() is
called and that will do 'hlist_del(&clk->clks_node)' which will touch in
already freed memory.

Fixes: 1df4046a93e08 ("clk: Combine __clk_get() and __clk_create_clk()")
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/clk/clk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ed119182aa1b..e82c3ee1da13 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4198,6 +4198,7 @@ void clk_unregister(struct clk *clk)
 		pr_warn("%s: unregistering protected clock: %s\n",
 					__func__, clk->core->name);
 
+	clk_core_unlink_consumer(clk);
 	kref_put(&clk->core->ref, __clk_release);
 	free_clk(clk);
 unlock:
-- 
2.35.1

Re: [RFC PATCH 2/4] clk: fix clk not being unlinked from consumers list

[Stephen Boyd]

Quoting Nuno Sá (2022-04-07 06:30:34)
> When a clk_hw is resgistered we add a struct clk handle to it's

s/resgistered/registered/

> consumers list.

Please add that the clk handle is created in __clk_register() per the
alloc_clk() call.


> Hence, we need to remove it when unregistering the
> clk_hw. This could actually lead to a use after free if a provider get's

s/get's/gets/

> removed before a consumer. When removing the consumer, __clk_put() is
> called and that will do 'hlist_del(&clk->clks_node)' which will touch in
> already freed memory.

Did this actually happen? I don't see how __clk_put() is called on the
internal hw->clk pointer. This pointer in hw->clk should be removed but
so far we've kept it around and various clk providers have used it. If
we start removing it now I'm not sure it will work because we would
probably expose many dangling pointer problems.

> 
> Fixes: 1df4046a93e08 ("clk: Combine __clk_get() and __clk_create_clk()")
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/clk/clk.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index ed119182aa1b..e82c3ee1da13 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4198,6 +4198,7 @@ void clk_unregister(struct clk *clk)
>                 pr_warn("%s: unregistering protected clock: %s\n",
>                                         __func__, clk->core->name);
>  
> +       clk_core_unlink_consumer(clk);
>         kref_put(&clk->core->ref, __clk_release);
>         free_clk(clk);
>  unlock:
> -- 
> 2.35.1
>

RE: [RFC PATCH 2/4] clk: fix clk not being unlinked from consumers list

[Sa, Nuno]

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Friday, April 22, 2022 2:20 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>; linux-clk@vger.kernel.org
> Cc: Michael Turquette <mturquette@baylibre.com>
> Subject: Re: [RFC PATCH 2/4] clk: fix clk not being unlinked from
> consumers list
> 
> [External]
> 
> Quoting Nuno Sá (2022-04-07 06:30:34)
> > When a clk_hw is resgistered we add a struct clk handle to it's
> 
> s/resgistered/registered/
> 
> > consumers list.
> 
> Please add that the clk handle is created in __clk_register() per the
> alloc_clk() call.
> 
> 
> > Hence, we need to remove it when unregistering the
> > clk_hw. This could actually lead to a use after free if a provider get's
> 
> s/get's/gets/
> 
> > removed before a consumer. When removing the consumer,
> __clk_put() is
> > called and that will do 'hlist_del(&clk->clks_node)' which will touch in
> > already freed memory.
> 
> Did this actually happen?

Yes, but as I stated in the cover (I think), this only happens if users do
dumb things like removing the clock provider (let's say with an explicit
sysfs unbind or DYNAMIC_OF) before the consumer. In that case, we
do free_clk(clk) in clk_unregister() but we still keep it in hlist. Hence
by the time we unbind the consumer, we'll have memory corruption.
I could really see the  memory corruption dump with SLAB_DEBUG.

> I don't see how __clk_put() is called on the
> internal hw->clk pointer. This pointer in hw->clk should be removed
> but

It isn't, but __clk_put() will call hlist_del(&clk->clks_node) which will
touch the dangling hw->clk pointer. I'm don't really know the history
to know why we add the hw->clk to the consumers list. One alternative
would be to not add it at all? 

> so far we've kept it around and various clk providers have used it. If
> we start removing it now I'm not sure it will work because we would
> probably expose many dangling pointer problems.
> 

Don't think it will be an issue on providers. Note that on __clk_register(),
we do clk_core_link_consumer(), so it is only natural (at first glance at least)
that we do clk_core_unlink_consumer() on clk_unregister() (at this point the
provider is gone anyways). Note that this was actually done like this prio
 the commit in the Fixes tag.

- Nuno Sá

[RFC PATCH 3/4] clk: refcount the active parent clk_core

[Nuno Sá]

As we keep a reference of the parent clk_hw, we should refcount it.
Otherwise, we could end up with a use after free situation. With
the following topology:

         providers              |   consumer
+----------+     +----------+   |   +-------+
| clk_hw A | --> | clk_hw B | <---- | clk C |
+----------+     +----------+   |   +-------+

Being clk_hw A and B provided by the same device, removing this device
will effectively leave clk_core B with a pointer to clk_core C which was
freed (clk C holds a reference to B and hence B won't be freed).
Thus, when we do remove the clk consumer, bad things can (and will)
happen.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/clk/clk.c | 80 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e82c3ee1da13..a2d8778ca3e0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1851,6 +1851,47 @@ static void __clk_set_parent_after(struct clk_core *core,
 	}
 }
 
+static void clk_core_free_parent_map(struct clk_core *core)
+{
+	int i = core->num_parents;
+
+	if (!core->num_parents)
+		return;
+
+	while (--i >= 0) {
+		kfree_const(core->parents[i].name);
+		kfree_const(core->parents[i].fw_name);
+	}
+
+	kfree(core->parents);
+}
+
+/* Free memory allocated for a clock. */
+static void __clk_release(struct kref *ref)
+{
+	struct clk_core *core = container_of(ref, struct clk_core, ref);
+
+	lockdep_assert_held(&prepare_lock);
+
+	if (core->parent)
+		kref_put(&core->parent->ref, __clk_release);
+
+	clk_core_free_parent_map(core);
+	kfree_const(core->name);
+	kfree(core);
+}
+
+/* deal with new, old parent references */
+static void __clk_reparent_refs_update(struct clk_core *new_parent,
+				       struct clk_core *old_parent)
+{
+	if (new_parent)
+		kref_get(&new_parent->ref);
+
+	if (old_parent)
+		kref_put(&old_parent->ref, __clk_release);
+}
+
 static int __clk_set_parent(struct clk_core *core, struct clk_core *parent,
 			    u8 p_index)
 {
@@ -1878,6 +1919,7 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent,
 	}
 
 	__clk_set_parent_after(core, parent, old_parent);
+	__clk_reparent_refs_update(parent, old_parent);
 
 	return 0;
 }
@@ -2118,6 +2160,7 @@ static void clk_change_rate(struct clk_core *core)
 
 		trace_clk_set_parent_complete(core, core->new_parent);
 		__clk_set_parent_after(core, core->new_parent, old_parent);
+		__clk_reparent_refs_update(core->new_parent, old_parent);
 	}
 
 	if (core->flags & CLK_OPS_PARENT_ENABLE)
@@ -3477,6 +3520,7 @@ static void clk_core_reparent_orphans_nolock(void)
 			/* update the clk tree topology */
 			__clk_set_parent_before(orphan, parent);
 			__clk_set_parent_after(orphan, parent, NULL);
+			__clk_reparent_refs_update(parent, NULL);
 			__clk_recalc_accuracies(orphan);
 			__clk_recalc_rates(orphan, 0);
 
@@ -3599,6 +3643,7 @@ static int __clk_core_init(struct clk_core *core)
 	if (parent) {
 		hlist_add_head(&core->child_node, &parent->children);
 		core->orphan = parent->orphan;
+		kref_get(&parent->ref);
 	} else if (!core->num_parents) {
 		hlist_add_head(&core->child_node, &clk_root_list);
 		core->orphan = false;
@@ -3677,10 +3722,14 @@ static int __clk_core_init(struct clk_core *core)
 		}
 	}
 
-	clk_core_reparent_orphans_nolock();
+	/*
+	 * Some orphan might be reparented to us grabbing a reference. Hence,
+	 * this has to be initialized now.
+	 */
+	kref_init(&core->ref);
 
+	clk_core_reparent_orphans_nolock();
 
-	kref_init(&core->ref);
 out:
 	clk_pm_runtime_put(core);
 unlock:
@@ -3894,21 +3943,6 @@ static int clk_core_populate_parent_map(struct clk_core *core,
 	return 0;
 }
 
-static void clk_core_free_parent_map(struct clk_core *core)
-{
-	int i = core->num_parents;
-
-	if (!core->num_parents)
-		return;
-
-	while (--i >= 0) {
-		kfree_const(core->parents[i].name);
-		kfree_const(core->parents[i].fw_name);
-	}
-
-	kfree(core->parents);
-}
-
 static struct clk *
 __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 {
@@ -4068,18 +4102,6 @@ int of_clk_hw_register(struct device_node *node, struct clk_hw *hw)
 }
 EXPORT_SYMBOL_GPL(of_clk_hw_register);
 
-/* Free memory allocated for a clock. */
-static void __clk_release(struct kref *ref)
-{
-	struct clk_core *core = container_of(ref, struct clk_core, ref);
-
-	lockdep_assert_held(&prepare_lock);
-
-	clk_core_free_parent_map(core);
-	kfree_const(core->name);
-	kfree(core);
-}
-
 /*
  * Empty clk_ops for unregistered clocks. These are used temporarily
  * after clk_unregister() was called on a clock and until last clock
-- 
2.35.1

[RFC PATCH 4/4] clk: use clk_core_unlink_consumer() helper

[Nuno Sá]

There is an helper to remove a consumer from the clk provider list.
Hence, let's use it when releasing a consumer.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a2d8778ca3e0..a9302bb7b92e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4425,7 +4425,7 @@ void __clk_put(struct clk *clk)
 		clk->exclusive_count = 0;
 	}
 
-	hlist_del(&clk->clks_node);
+	clk_core_unlink_consumer(clk);
 	if (clk->min_rate > clk->core->req_rate ||
 	    clk->max_rate < clk->core->req_rate)
 		clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
-- 
2.35.1