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

[RESEND RFC PATCH v2 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...

v2:
 * add an helper function in '__set_clk_parents()' to set each parent
separately;
 * massaged commit message in patch 2/4
("clk: fix clk not being unlinked from consumers list") as per Stephen
feedback.

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 | 126 +++++++++++++++++++++++++----------------
 drivers/clk/clk.c      |  83 +++++++++++++++++----------
 2 files changed, 131 insertions(+), 78 deletions(-)

-- 
2.17.1

[RESEND RFC PATCH v2 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.

In order to make things easier to follow, an helper function was added
to set each parent in it's own function.

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

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 2ef819606c41..b8ea4f93f35e 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -11,11 +11,78 @@
 #include <linux/of.h>
 #include <linux/printk.h>
 
-static int __set_clk_parents(struct device_node *node, bool clk_supplier)
+static int __set_clk_parent(struct device_node *node, bool clk_supplier,
+			    int index, bool *stop)
 {
 	struct of_phandle_args clkspec;
-	int index, rc, num_parents;
 	struct clk *clk, *pclk;
+	int rc;
+
+	rc = of_parse_phandle_with_args(node, "assigned-clock-parents",
+					"#clock-cells",	index, &clkspec);
+	if (rc) {
+		/* skip empty (null) phandles */
+		if (rc == -ENOENT)
+			return 0;
+
+		return rc;
+	}
+
+	if (clkspec.np == node && !clk_supplier) {
+		*stop = true;
+		goto out_of_put;
+	}
+
+	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",
+				index, node);
+
+		return PTR_ERR(pclk);
+	}
+
+	rc = of_parse_phandle_with_args(node, "assigned-clocks",
+					"#clock-cells", index, &clkspec);
+	if (rc) {
+		clk_put(pclk);
+		return rc;
+	}
+
+	if (clkspec.np == node && !clk_supplier) {
+		*stop = true;
+		goto out_clk_put;
+	}
+
+	clk = of_clk_get_from_provider(&clkspec);
+	if (IS_ERR(clk)) {
+		if (PTR_ERR(clk) != -EPROBE_DEFER)
+			pr_warn("clk: couldn't get assigned clock %d for %pOF\n",
+				index, node);
+
+		rc = PTR_ERR(clk);
+		goto out_clk_put;
+	}
+
+	rc = clk_set_parent(clk, pclk);
+	if (rc)
+		pr_err("clk: failed to reparent %s to %s: %d\n",
+		       __clk_get_name(clk), __clk_get_name(pclk), rc);
+
+	clk_put(clk);
+
+out_clk_put:
+	clk_put(pclk);
+out_of_put:
+	of_node_put(clkspec.np);
+	return rc;
+}
+
+static int __set_clk_parents(struct device_node *node, bool clk_supplier)
+{
+	int index, rc, num_parents;
+	bool stop = false;
 
 	num_parents = of_count_phandle_with_args(node, "assigned-clock-parents",
 						 "#clock-cells");
@@ -24,53 +91,12 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 		       node);
 
 	for (index = 0; index < num_parents; index++) {
-		rc = of_parse_phandle_with_args(node, "assigned-clock-parents",
-					"#clock-cells",	index, &clkspec);
-		if (rc < 0) {
-			/* skip empty (null) phandles */
-			if (rc == -ENOENT)
-				continue;
-			else
-				return rc;
-		}
-		if (clkspec.np == node && !clk_supplier)
-			return 0;
-		pclk = of_clk_get_from_provider(&clkspec);
-		if (IS_ERR(pclk)) {
-			if (PTR_ERR(pclk) != -EPROBE_DEFER)
-				pr_warn("clk: couldn't get parent clock %d for %pOF\n",
-					index, node);
-			return PTR_ERR(pclk);
-		}
-
-		rc = of_parse_phandle_with_args(node, "assigned-clocks",
-					"#clock-cells", index, &clkspec);
-		if (rc < 0)
-			goto err;
-		if (clkspec.np == node && !clk_supplier) {
-			rc = 0;
-			goto err;
-		}
-		clk = of_clk_get_from_provider(&clkspec);
-		if (IS_ERR(clk)) {
-			if (PTR_ERR(clk) != -EPROBE_DEFER)
-				pr_warn("clk: couldn't get assigned clock %d for %pOF\n",
-					index, node);
-			rc = PTR_ERR(clk);
-			goto err;
-		}
-
-		rc = clk_set_parent(clk, pclk);
-		if (rc < 0)
-			pr_err("clk: failed to reparent %s to %s: %d\n",
-			       __clk_get_name(clk), __clk_get_name(pclk), rc);
-		clk_put(clk);
-		clk_put(pclk);
+		rc = __set_clk_parent(node, clk_supplier, index, &stop);
+		if (rc || stop)
+			return rc;
 	}
+
 	return 0;
-err:
-	clk_put(pclk);
-	return rc;
 }
 
 static int __set_clk_rates(struct device_node *node, bool clk_supplier)
@@ -93,14 +119,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 +139,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.17.1

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

[Nuno Sá]

When a clk_hw is registered we add a struct clk handle to it's
consumers list. This handle is created in '__clk_register()' per the
'alloc_clk()' call.

As such, we need to remove this handle when unregistering the
clk_hw. This can actually lead to a use after free if a provider 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.

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 7fc191c15507..fdc711e53fb7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4191,6 +4191,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.17.1

[RESEND RFC PATCH v2 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 fdc711e53fb7..92625f1523f5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1844,6 +1844,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)
 {
@@ -1871,6 +1912,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;
 }
@@ -2111,6 +2153,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)
@@ -3470,6 +3513,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);
 
@@ -3592,6 +3636,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;
@@ -3670,10 +3715,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:
@@ -3887,21 +3936,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)
 {
@@ -4061,18 +4095,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.17.1

[RESEND RFC PATCH v2 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 92625f1523f5..9eb5a7c2c73b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4370,7 +4370,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.17.1

Re: [RESEND RFC PATCH v2 0/4] Dynamic OF and use after free related fixes

[Nuno Sá]

On Thu, 2022-06-23 at 14:18 +0200, Nuno Sá wrote:
> 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...
> 
> v2:
>  * add an helper function in '__set_clk_parents()' to set each parent
> separately;
>  * massaged commit message in patch 2/4
> ("clk: fix clk not being unlinked from consumers list") as per
> Stephen
> feedback.
> 
> 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 | 126 +++++++++++++++++++++++++--------------
> --
>  drivers/clk/clk.c      |  83 +++++++++++++++++----------
>  2 files changed, 131 insertions(+), 78 deletions(-)
> 

Hi Michael and Stephen,

Is there anything missing on this one? I understand that maintainers
are very busy people but this is already a RESEND and I already waited
a fair amount of time before deciding for this mail.

Let me know if there's anything for me to do or if the series is just
something you are not keen to apply (even though I think the series
makes sense and makes the code more robust).

- Nuno Sá

Re: [RESEND RFC PATCH v2 0/4] Dynamic OF and use after free related fixes

[Nuno Sá]

On Wed, 2022-07-13 at 15:24 +0200, Nuno Sá wrote:
> On Thu, 2022-06-23 at 14:18 +0200, Nuno Sá wrote:
> > 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...
> > 
> > v2:
> >  * add an helper function in '__set_clk_parents()' to set each
> > parent
> > separately;
> >  * massaged commit message in patch 2/4
> > ("clk: fix clk not being unlinked from consumers list") as per
> > Stephen
> > feedback.
> > 
> > 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 | 126 +++++++++++++++++++++++++------------
> > --
> > --
> >  drivers/clk/clk.c      |  83 +++++++++++++++++----------
> >  2 files changed, 131 insertions(+), 78 deletions(-)
> > 
> 
> Hi Michael and Stephen,
> 
> Is there anything missing on this one? I understand that maintainers
> are very busy people but this is already a RESEND and I already
> waited
> a fair amount of time before deciding for this mail.
> 
> Let me know if there's anything for me to do or if the series is just
> something you are not keen to apply (even though I think the series
> makes sense and makes the code more robust).
> 
> - Nuno Sá

Pinging this one more time... I'm happy to rebase and resend but it
would be nice to have some feedback. Otherwise I'll just forget about
this which is, honesty, very sad since I did spent some time on it :)
and it is fixing some real bugs (even though in reality are very hard
to trigger).

- Nuno Sá