linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
@ 2021-04-06 23:06 Dmitry Baryshkov
  2021-04-07  9:00 ` Dmitry Baryshkov
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-04-06 23:06 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Stephen Boyd, Michael Turquette
  Cc: linux-arm-msm, linux-clk, dri-devel, freedreno, Rob Clark, Daniel Palmer

devm_clk_hw_register_fixed_factor_release(), the release function for
the devm_clk_hw_register_fixed_factor(), calls
clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
after that the devres functions will also kfree the allocated data,
resulting in double free/memory corruption. Just call
clk_hw_unregister() instead, leaving kfree() to devres code.

Reported-by: Rob Clark <robdclark@chromium.org>
Cc: Daniel Palmer <daniel@0x0f.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---

Stephen, this fix affects the DSI PHY rework. Do we have a chance of
getting it into 5.12, otherwise there will be a cross-dependency between
msm-next and clk-next.

---
 drivers/clk/clk-fixed-factor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 4f7bf3929d6d..390c16f321a6 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -66,7 +66,12 @@ EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
 
 static void devm_clk_hw_register_fixed_factor_release(struct device *dev, void *res)
 {
-	clk_hw_unregister_fixed_factor(&((struct clk_fixed_factor *)res)->hw);
+	/*
+	 * We can not use clk_hw_unregister_fixed_factor, since it will kfree()
+	 * the hw, resulting in double free. Just unregister the hw and let
+	 * devres code kfree() it.
+	 */
+	clk_hw_unregister(&((struct clk_fixed_factor *)res)->hw);
 }
 
 static struct clk_hw *
-- 
2.30.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
  2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
@ 2021-04-07  9:00 ` Dmitry Baryshkov
  2021-04-07 11:38 ` Daniel Palmer
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-04-07  9:00 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Stephen Boyd, Michael Turquette
  Cc: open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:COMMON CLK FRAMEWORK,
	open list:DRM DRIVER FOR MSM ADRENO GPU, freedreno, Rob Clark,
	Daniel Palmer

On Wed, 7 Apr 2021 at 02:06, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> devm_clk_hw_register_fixed_factor_release(), the release function for
> the devm_clk_hw_register_fixed_factor(), calls
> clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> after that the devres functions will also kfree the allocated data,
> resulting in double free/memory corruption. Just call
> clk_hw_unregister() instead, leaving kfree() to devres code.
>
> Reported-by: Rob Clark <robdclark@chromium.org>
> Cc: Daniel Palmer <daniel@0x0f.com>

Forgot:

Fixes: 0b9266d295ce ("clk: fixed: add devm helper for
clk_hw_register_fixed_factor()")


> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>
> Stephen, this fix affects the DSI PHY rework. Do we have a chance of
> getting it into 5.12, otherwise there will be a cross-dependency between
> msm-next and clk-next.
>
> ---
>  drivers/clk/clk-fixed-factor.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
  2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
  2021-04-07  9:00 ` Dmitry Baryshkov
@ 2021-04-07 11:38 ` Daniel Palmer
  2021-04-07 22:41 ` Stephen Boyd
  2021-04-07 23:01 ` Stephen Boyd
  3 siblings, 0 replies; 8+ messages in thread
From: Daniel Palmer @ 2021-04-07 11:38 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Stephen Boyd, Michael Turquette,
	linux-arm-msm, linux-clk, dri-devel, freedreno, Rob Clark

Hi Dmitry,

On Wed, 7 Apr 2021 at 08:06, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> devm_clk_hw_register_fixed_factor_release(), the release function for
> the devm_clk_hw_register_fixed_factor(), calls
> clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> after that the devres functions will also kfree the allocated data,
> resulting in double free/memory corruption. Just call
> clk_hw_unregister() instead, leaving kfree() to devres code.

Doh.
Sorry for not spotting this when I wrote the patch.
Thank you for cleaning up after me.

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
  2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
  2021-04-07  9:00 ` Dmitry Baryshkov
  2021-04-07 11:38 ` Daniel Palmer
@ 2021-04-07 22:41 ` Stephen Boyd
  2021-04-07 22:57   ` Dmitry Baryshkov
  2021-04-07 23:01 ` Stephen Boyd
  3 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2021-04-07 22:41 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Michael Turquette
  Cc: linux-arm-msm, linux-clk, dri-devel, freedreno, Rob Clark, Daniel Palmer

Quoting Dmitry Baryshkov (2021-04-06 16:06:06)
> devm_clk_hw_register_fixed_factor_release(), the release function for
> the devm_clk_hw_register_fixed_factor(), calls
> clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> after that the devres functions will also kfree the allocated data,
> resulting in double free/memory corruption. Just call
> clk_hw_unregister() instead, leaving kfree() to devres code.
> 
> Reported-by: Rob Clark <robdclark@chromium.org>
> Cc: Daniel Palmer <daniel@0x0f.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> 
> Stephen, this fix affects the DSI PHY rework. Do we have a chance of
> getting it into 5.12, otherwise there will be a cross-dependency between
> msm-next and clk-next.

Think I can get this into the last fixes PR. One question though, I
think this follows the pattern that things like clk-divider.c use for
devm. Are those also broken?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
  2021-04-07 22:41 ` Stephen Boyd
@ 2021-04-07 22:57   ` Dmitry Baryshkov
       [not found]     ` <161783803315.3790633.10829887417379757624@swboyd.mtv.corp.google.com>
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-04-07 22:57 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Bjorn Andersson, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:COMMON CLK FRAMEWORK,
	open list:DRM DRIVER FOR MSM ADRENO GPU, freedreno, Rob Clark,
	Daniel Palmer

On Thu, 8 Apr 2021 at 01:41, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2021-04-06 16:06:06)
> > devm_clk_hw_register_fixed_factor_release(), the release function for
> > the devm_clk_hw_register_fixed_factor(), calls
> > clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> > after that the devres functions will also kfree the allocated data,
> > resulting in double free/memory corruption. Just call
> > clk_hw_unregister() instead, leaving kfree() to devres code.
> >
> > Reported-by: Rob Clark <robdclark@chromium.org>
> > Cc: Daniel Palmer <daniel@0x0f.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >
> > Stephen, this fix affects the DSI PHY rework. Do we have a chance of
> > getting it into 5.12, otherwise there will be a cross-dependency between
> > msm-next and clk-next.
>
> Think I can get this into the last fixes PR. One question though, I
> think this follows the pattern that things like clk-divider.c use for
> devm. Are those also broken?

It looks so. See e.g. the devres_release() function. It calls
(*release) callback, then it will kfree the resource.
Also see Documentation/driver-api/driver-model/devres.rst, which does
not kfree() in release functions.

Do you wish for me to send all the fixes?



-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
  2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
                   ` (2 preceding siblings ...)
  2021-04-07 22:41 ` Stephen Boyd
@ 2021-04-07 23:01 ` Stephen Boyd
  3 siblings, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2021-04-07 23:01 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Michael Turquette
  Cc: linux-arm-msm, linux-clk, dri-devel, freedreno, Rob Clark, Daniel Palmer

Quoting Dmitry Baryshkov (2021-04-06 16:06:06)
> devm_clk_hw_register_fixed_factor_release(), the release function for
> the devm_clk_hw_register_fixed_factor(), calls
> clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> after that the devres functions will also kfree the allocated data,
> resulting in double free/memory corruption. Just call
> clk_hw_unregister() instead, leaving kfree() to devres code.
> 
> Reported-by: Rob Clark <robdclark@chromium.org>
> Cc: Daniel Palmer <daniel@0x0f.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

Applied to clk-fixes. I also squashed this in to cleanup that ugly cast.

---8<----

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 390c16f321a6..4e4b6d367612 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -66,12 +66,14 @@ EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
 
 static void devm_clk_hw_register_fixed_factor_release(struct device *dev, void *res)
 {
+	struct clk_fixed_factor *fix = res;
+
 	/*
 	 * We can not use clk_hw_unregister_fixed_factor, since it will kfree()
 	 * the hw, resulting in double free. Just unregister the hw and let
 	 * devres code kfree() it.
 	 */
-	clk_hw_unregister(&((struct clk_fixed_factor *)res)->hw);
+	clk_hw_unregister(&fix->hw);
 }
 
 static struct clk_hw *

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
       [not found]     ` <161783803315.3790633.10829887417379757624@swboyd.mtv.corp.google.com>
@ 2021-04-08  0:52       ` Dmitry Baryshkov
  2021-04-08  7:32         ` Stephen Boyd
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Baryshkov @ 2021-04-08  0:52 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Bjorn Andersson, Michael Turquette,
	DRM DRIVER FOR MSM ADRENO GPU,
	open list:COMMON CLK FRAMEWORK <linux-clk@vger.kernel.org>,
	open list:DRM DRIVER FOR MSM ADRENO GPU
	<dri-devel@lists.freedesktop.org>,
	freedreno <freedreno@lists.freedesktop.org>,
	Rob Clark <robdclark@chromium.org>,
	Daniel Palmer

On Thu, 8 Apr 2021 at 02:27, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2021-04-07 15:57:01)
> > On Thu, 8 Apr 2021 at 01:41, Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Dmitry Baryshkov (2021-04-06 16:06:06)
> > > > devm_clk_hw_register_fixed_factor_release(), the release function for
> > > > the devm_clk_hw_register_fixed_factor(), calls
> > > > clk_hw_unregister_fixed_factor(), which will kfree() the clock. However
> > > > after that the devres functions will also kfree the allocated data,
> > > > resulting in double free/memory corruption. Just call
> > > > clk_hw_unregister() instead, leaving kfree() to devres code.
> > > >
> > > > Reported-by: Rob Clark <robdclark@chromium.org>
> > > > Cc: Daniel Palmer <daniel@0x0f.com>
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >
> > > > Stephen, this fix affects the DSI PHY rework. Do we have a chance of
> > > > getting it into 5.12, otherwise there will be a cross-dependency between
> > > > msm-next and clk-next.
> > >
> > > Think I can get this into the last fixes PR. One question though, I
> > > think this follows the pattern that things like clk-divider.c use for
> > > devm. Are those also broken?
> >
> > It looks so. See e.g. the devres_release() function. It calls
> > (*release) callback, then it will kfree the resource.
> > Also see Documentation/driver-api/driver-model/devres.rst, which does
> > not kfree() in release functions.
> >
> > Do you wish for me to send all the fixes?
> >
>
> Yes please send more fixes. They're not high priority though so I'll
> probably leave them to bake in next for a week or so.

Short story: no other patches needed.

Long story:
I've checked the rest of devres allocations in clk subsystem.
Interesting, they use a bit different pattern: they devres_alloc a
pointer to the clock, then they fill the pointer with the new clock
data. The release callback would (correctly) free the clock pointer by
the devres and then devres code would kfree the devres data (clock
pointer).

The fixed-factor is unique in this area, because it devres_alloc's a
clock instance (rather than the pointer) and then fills it, so it
should not be freed in the release callback (only unregistered) with
the devres code kfreeing() the instance itself.


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
  2021-04-08  0:52       ` Dmitry Baryshkov
@ 2021-04-08  7:32         ` Stephen Boyd
  0 siblings, 0 replies; 8+ messages in thread
From: Stephen Boyd @ 2021-04-08  7:32 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Michael Turquette,
	DRM DRIVER FOR MSM ADRENO GPU,
	open list:COMMON CLK FRAMEWORK <linux-clk@vger.kernel.org>,
	open  list:DRM DRIVER FOR MSM ADRENO GPU
	<dri-devel@lists.freedesktop.org>,
	 freedreno <freedreno@lists.freedesktop.org>,
	Rob Clark  <robdclark@chromium.org>,
	Daniel Palmer

Quoting Dmitry Baryshkov (2021-04-07 17:52:01)
>
> Short story: no other patches needed.
> 
> Long story:
> I've checked the rest of devres allocations in clk subsystem.
> Interesting, they use a bit different pattern: they devres_alloc a
> pointer to the clock, then they fill the pointer with the new clock
> data. The release callback would (correctly) free the clock pointer by
> the devres and then devres code would kfree the devres data (clock
> pointer).
> 
> The fixed-factor is unique in this area, because it devres_alloc's a
> clock instance (rather than the pointer) and then fills it, so it
> should not be freed in the release callback (only unregistered) with
> the devres code kfreeing() the instance itself.
> 
> 

Cool thanks for checking. Maybe we should change those other callers in
clk directory to devres alloc the container structure instead of a
pointer. Then we avoid the double allocation. Glad it's not a critical
fix though.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-04-08  7:32 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 23:06 [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock Dmitry Baryshkov
2021-04-07  9:00 ` Dmitry Baryshkov
2021-04-07 11:38 ` Daniel Palmer
2021-04-07 22:41 ` Stephen Boyd
2021-04-07 22:57   ` Dmitry Baryshkov
     [not found]     ` <161783803315.3790633.10829887417379757624@swboyd.mtv.corp.google.com>
2021-04-08  0:52       ` Dmitry Baryshkov
2021-04-08  7:32         ` Stephen Boyd
2021-04-07 23:01 ` Stephen Boyd

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox