Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Michael Turquette <mturquette@baylibre.com>,
	DRM DRIVER FOR MSM ADRENO GPU  <linux-arm-msm@vger.kernel.org>,
	"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" <daniel@0x0f.com>
Subject: Re: [PATCH] clk: fixed: fix double free in resource managed fixed-factor clock
Date: Thu, 8 Apr 2021 03:52:01 +0300
Message-ID: <CAA8EJprrQVPZxV7nhScgTCL7ZKU2c1AicNjOvd2YUEu8pCUxkQ@mail.gmail.com> (raw)
In-Reply-To: <161783803315.3790633.10829887417379757624@swboyd.mtv.corp.google.com>

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

  parent reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-06 23:06 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 [this message]
2021-04-08  7:32         ` Stephen Boyd
2021-04-07 23:01 ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAA8EJprrQVPZxV7nhScgTCL7ZKU2c1AicNjOvd2YUEu8pCUxkQ@mail.gmail.com \
    --to=dmitry.baryshkov@linaro.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel@0x0f.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git