[PATCH 0/3] Fix for drm_mode_config_cleanup issue

[PATCH 0/3] Fix for drm_mode_config_cleanup issue

[Michael Grzeschik]

Fix the issue: "drm/imx: Crash in drm_mode_config_cleanup".

Which was documented here:

https://www.spinics.net/lists/dri-devel/msg189388.html

And tried to address with this:

https://patchwork.kernel.org/patch/10633297/
https://patchwork.kernel.org/patch/10633299/

This series fixes the possible case of use after free by adding action
functions to the devres cleanup path. So it will ensure the generic
cleanup code will not use the already freed memory.


Michael Grzeschik (3):
  ipuv3-crtc: add remove action for devres data
  ipuv3-ldb: add init list head on bind
  ipuv3-ldb: add remove action for devres data

 drivers/gpu/drm/imx/imx-ldb.c    | 35 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/ipuv3-crtc.c | 12 +++++++++++
 2 files changed, 47 insertions(+)

-- 
2.20.1

[PATCH 1/3] ipuv3-crtc: add remove action for devres data

[Michael Grzeschik]

The destroy function in drm_mode_config_cleanup will remove the objects
in ipu-drm-core by calling its destroy functions if the bind function
fails. The drm_crtc is also part of the devres allocated ipu_crtc
object. The ipu_crtc object will already be cleaned up if the bind for
the crtc fails. This leads drm_crtc_cleanup try to clean already freed
memory.

We fix this issue by adding the devres action ipu_crtc_remove_head which
will remove its head from the objects in ipu-drm-core which then never
calls its destroy function anymore.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index ec3602ebbc1cd..fa1ee33a43d77 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -429,6 +429,14 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 	return ret;
 }
 
+static void ipu_crtc_remove_head(void *data)
+{
+	struct ipu_crtc *ipu_crtc = data;
+	struct drm_crtc *crtc = &ipu_crtc->base;
+
+	list_del(&crtc->head);
+}
+
 static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
 {
 	struct ipu_client_platformdata *pdata = dev->platform_data;
@@ -440,6 +448,10 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
 	if (!ipu_crtc)
 		return -ENOMEM;
 
+	ret = devm_add_action(dev, ipu_crtc_remove_head, ipu_crtc);
+	if (ret)
+		return ret;
+
 	ipu_crtc->dev = dev;
 
 	ret = ipu_crtc_init(ipu_crtc, pdata, drm);
-- 
2.20.1

Re: [PATCH 1/3] ipuv3-crtc: add remove action for devres data

[Philipp Zabel]

Hi Michael,

On Tue, 2019-04-02 at 15:49 +0200, Michael Grzeschik wrote:
> The destroy function in drm_mode_config_cleanup will remove the objects
> in ipu-drm-core by calling its destroy functions if the bind function
> fails. The drm_crtc is also part of the devres allocated ipu_crtc
> object. The ipu_crtc object will already be cleaned up if the bind for
> the crtc fails. This leads drm_crtc_cleanup try to clean already freed
> memory.
> 
> We fix this issue by adding the devres action ipu_crtc_remove_head which
> will remove its head from the objects in ipu-drm-core which then never
> calls its destroy function anymore.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/ipuv3-crtc.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index ec3602ebbc1cd..fa1ee33a43d77 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -429,6 +429,14 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
>  	return ret;
>  }
>  
> +static void ipu_crtc_remove_head(void *data)
> +{
> +	struct ipu_crtc *ipu_crtc = data;
> +	struct drm_crtc *crtc = &ipu_crtc->base;
> +
> +	list_del(&crtc->head);

I don't think reaching into drm_crtc internals like this is going to be
robust. Currently, this is either missing the rest of drm_crtc_cleanup,
or it will crash if drm_crtc_init_with_planes hasn't been called yet.

I think you could call devm_add_action with a function that calls
drm_crtc_cleanup after drm_crtc_init_with_planes in ipu_crtc_init.

Alternatively, the ipu_crtc allocation could be changed to kzalloc. It
would then have to freed manually in the drm_crtc_funcs->destroy
callback.

regards
Philipp

Re: [PATCH 1/3] ipuv3-crtc: add remove action for devres data

[Daniel Vetter]

On Tue, Apr 02, 2019 at 05:49:23PM +0200, Philipp Zabel wrote:
> Hi Michael,
> 
> On Tue, 2019-04-02 at 15:49 +0200, Michael Grzeschik wrote:
> > The destroy function in drm_mode_config_cleanup will remove the objects
> > in ipu-drm-core by calling its destroy functions if the bind function
> > fails. The drm_crtc is also part of the devres allocated ipu_crtc
> > object. The ipu_crtc object will already be cleaned up if the bind for
> > the crtc fails. This leads drm_crtc_cleanup try to clean already freed
> > memory.
> > 
> > We fix this issue by adding the devres action ipu_crtc_remove_head which
> > will remove its head from the objects in ipu-drm-core which then never
> > calls its destroy function anymore.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/gpu/drm/imx/ipuv3-crtc.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > index ec3602ebbc1cd..fa1ee33a43d77 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > @@ -429,6 +429,14 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
> >  	return ret;
> >  }
> >  
> > +static void ipu_crtc_remove_head(void *data)
> > +{
> > +	struct ipu_crtc *ipu_crtc = data;
> > +	struct drm_crtc *crtc = &ipu_crtc->base;
> > +
> > +	list_del(&crtc->head);
> 
> I don't think reaching into drm_crtc internals like this is going to be
> robust. Currently, this is either missing the rest of drm_crtc_cleanup,
> or it will crash if drm_crtc_init_with_planes hasn't been called yet.
> 
> I think you could call devm_add_action with a function that calls
> drm_crtc_cleanup after drm_crtc_init_with_planes in ipu_crtc_init.
> 
> Alternatively, the ipu_crtc allocation could be changed to kzalloc. It
> would then have to freed manually in the drm_crtc_funcs->destroy
> callback.

Dirty secret of devm: You can't use it for any drm_ structure, because the
lifetimes of those do not match the lifetimes of the underlying device.
We'd need to tie the lifetimes of drm objects to drm_device. Noralf has
some patches to move that forward. We'd need something like
drm_dev_kzalloc which releases memory when drm_device is freed.

Agreed that digging even deeper into the devm deadend is not a good idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 2/3] ipuv3-ldb: add init list head on bind

[Michael Grzeschik]

The list head is not initialised after imx_ldb_bind
allocates the imx_ldb object. Ensure it will be valid
before anyone uses it.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 3837333022804..a11f8758da70e 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -581,6 +581,20 @@ static int imx_ldb_panel_ddc(struct device *dev,
 	return 0;
 }
 
+static void ipu_ldb_init_encoder_head(struct imx_ldb *imx_ldb)
+{
+	struct imx_ldb_channel *imx_ldb_ch;
+	struct drm_encoder *encoder;
+
+	imx_ldb_ch = &imx_ldb->channel[0];
+	encoder = &imx_ldb_ch->encoder;
+	INIT_LIST_HEAD(&encoder->head);
+
+	imx_ldb_ch = &imx_ldb->channel[1];
+	encoder = &imx_ldb_ch->encoder;
+	INIT_LIST_HEAD(&encoder->head);
+}
+
 static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
@@ -597,6 +611,8 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 	if (!imx_ldb)
 		return -ENOMEM;
 
+	ipu_ldb_init_encoder_head(imx_ldb);
+
 	imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
 	if (IS_ERR(imx_ldb->regmap)) {
 		dev_err(dev, "failed to get parent regmap\n");
-- 
2.20.1

[PATCH 3/3] ipuv3-ldb: add remove action for devres data

[Michael Grzeschik]

The destroy function in drm_mode_config_cleanup will remove the objects
in ipu-drm-core by calling its destroy functions if the bind function
fails. The drm_encoder is also part of the devres allocated ipu_ldb
object. The ipu_ldb object will already be cleaned up if the bind for
the crtc fails. This leads drm_encoder_cleanup try to clean already freed
memory.

We fix this issue by adding the devres action ipu_ldb_remove_head which
will remove its head from the objects in ipu-drm-core which then never
calls its destroy function anymore.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index a11f8758da70e..0a224036cc68d 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -581,6 +581,21 @@ static int imx_ldb_panel_ddc(struct device *dev,
 	return 0;
 }
 
+static void ipu_ldb_remove_head(void *data)
+{
+	struct imx_ldb *imx_ldb = data;
+	struct imx_ldb_channel *imx_ldb_ch;
+	struct drm_encoder *encoder;
+
+	imx_ldb_ch = &imx_ldb->channel[0];
+	encoder = &imx_ldb_ch->encoder;
+	list_del_init(&encoder->head);
+
+	imx_ldb_ch = &imx_ldb->channel[1];
+	encoder = &imx_ldb_ch->encoder;
+	list_del_init(&encoder->head);
+}
+
 static void ipu_ldb_init_encoder_head(struct imx_ldb *imx_ldb)
 {
 	struct imx_ldb_channel *imx_ldb_ch;
@@ -613,6 +628,10 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 
 	ipu_ldb_init_encoder_head(imx_ldb);
 
+	ret = devm_add_action(dev, ipu_ldb_remove_head, imx_ldb);
+	if (ret)
+		return ret;
+
 	imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
 	if (IS_ERR(imx_ldb->regmap)) {
 		dev_err(dev, "failed to get parent regmap\n");
-- 
2.20.1