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

["Sa, Nuno" <Nuno.Sa@analog.com>]

> -----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á