[PATCH] clk: fixed: fix double free in resource managed fixed-factor clock

[PATCH] clk: fixed: fix double free in resource managed fixed-factor clock

[Dmitry Baryshkov]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock

[Dmitry Baryshkov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock

[Daniel Palmer]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock

[Stephen Boyd]

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?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock

[Dmitry Baryshkov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock

[Stephen Boyd]

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 *
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock

[Stephen Boyd]

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel