| Message ID | 20220407133036.213217-3-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Dynamic OF and use after free related fixes | expand |
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 >
> -----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á
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:
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(+)