| Message ID | 20210406230606.3007138-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | Accepted |
| Commit | 50ce6826a48f119baf2794fa384a64efe9bd84a5 |
| Headers | show |
| Series | clk: fixed: fix double free in resource managed fixed-factor clock | expand |
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
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
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
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 *
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(-) -- 2.30.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel