Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] drm/imx: fix memory leak in imx_pd_bind
@ 2019-10-04 19:09 Navid Emamdoost
  2019-10-05 15:02 ` Markus Elfring
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Navid Emamdoost @ 2019-10-04 19:09 UTC (permalink / raw)
  Cc: Daniel Vetter, David Airlie, Fabio Estevam, Sascha Hauer, kjlu,
	linux-kernel, dri-devel, emamd001, NXP Linux Team, Philipp Zabel,
	smccaman, Shawn Guo, Pengutronix Kernel Team, linux-arm-kernel,
	Navid Emamdoost

In imx_pd_bind, the duplicated memory for imxpd->edid via kmemdup should
be released in drm_of_find_panel_or_bridge or imx_pd_register fail.

Fixes: ebc944613567 ("drm: convert drivers to use drm_of_find_panel_or_bridge")
Fixes: 19022aaae677 ("staging: drm/imx: Add parallel display support")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/gpu/drm/imx/parallel-display.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index e7ce17503ae1..9522d2cb0ad5 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -227,14 +227,18 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 
 	/* port@1 is the output port */
 	ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
-	if (ret && ret != -ENODEV)
+	if (ret && ret != -ENODEV) {
+		kfree(imxpd->edid);
 		return ret;
+	}
 
 	imxpd->dev = dev;
 
 	ret = imx_pd_register(drm, imxpd);
-	if (ret)
+	if (ret) {
+		kfree(imxpd->edid);
 		return ret;
+	}
 
 	dev_set_drvdata(dev, imxpd);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] drm/imx: fix memory leak in imx_pd_bind
  2019-10-04 19:09 [PATCH] drm/imx: fix memory leak in imx_pd_bind Navid Emamdoost
@ 2019-10-05 15:02 ` Markus Elfring
  2019-10-12 11:54   ` Markus Elfring
  2019-10-06  9:33 ` drm/imx: Checking a kmemdup() call in imx_pd_bind() Markus Elfring
  2019-11-21 18:31 ` [PATCH] drm/imx: fix memory leak in imx_pd_bind Navid Emamdoost
  2 siblings, 1 reply; 15+ messages in thread
From: Markus Elfring @ 2019-10-05 15:02 UTC (permalink / raw)
  To: Navid Emamdoost, dri-devel, linux-arm-kernel, linux-imx, kernel
  Cc: kernel-janitors, Philipp Zabel, David Airlie, Shawn Guo,
	Sascha Hauer, Kangjie Lu, linux-kernel, Navid Emamdoost,
	Daniel Vetter, Stephen McCamant, Fabio Estevam

> In imx_pd_bind, the duplicated memory for imxpd->edid via kmemdup should
> be released in drm_of_find_panel_or_bridge or imx_pd_register fail.

Please improve this change description.


> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -227,14 +227,18 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>
>  	/* port@1 is the output port */
>  	ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
> -	if (ret && ret != -ENODEV)
> +	if (ret && ret != -ENODEV) {
> +		kfree(imxpd->edid);
>  		return ret;
> +	}
>
>  	imxpd->dev = dev;

Please use a jump target here instead of adding duplicate source code
for the completion of exception handling.

 	if (ret && ret != -ENODEV)
-		return ret;
+		goto free_edid;

…

+free_edid:
+ 	kfree(imxpd->edid);
+ 	return ret;


Regards,
Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: drm/imx: Checking a kmemdup() call in imx_pd_bind()
  2019-10-04 19:09 [PATCH] drm/imx: fix memory leak in imx_pd_bind Navid Emamdoost
  2019-10-05 15:02 ` Markus Elfring
@ 2019-10-06  9:33 ` Markus Elfring
  2019-10-07  4:26   ` Navid Emamdoost
  2019-11-21 18:31 ` [PATCH] drm/imx: fix memory leak in imx_pd_bind Navid Emamdoost
  2 siblings, 1 reply; 15+ messages in thread
From: Markus Elfring @ 2019-10-06  9:33 UTC (permalink / raw)
  To: Navid Emamdoost, dri-devel, kernel, linux-arm-kernel, linux-imx
  Cc: kernel-janitors, Philipp Zabel, David Airlie, Shawn Guo,
	Sascha Hauer, Kangjie Lu, LKML, Navid Emamdoost, Daniel Vetter,
	Stephen McCamant, Fabio Estevam

I have taken another look also at the implementation of the function “imx_pd_bind”.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/imx/parallel-display.c?id=43b815c6a8e7dbccb5b8bd9c4b099c24bc22d135#n197
https://elixir.bootlin.com/linux/v5.4-rc1/source/drivers/gpu/drm/imx/parallel-display.c#L197

Now I find an unchecked call of the function “kmemdup” suspicious.
Will this detail trigger further software development considerations?

Regards,
Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: drm/imx: Checking a kmemdup() call in imx_pd_bind()
  2019-10-06  9:33 ` drm/imx: Checking a kmemdup() call in imx_pd_bind() Markus Elfring
@ 2019-10-07  4:26   ` Navid Emamdoost
  2019-10-07  7:44     ` Markus Elfring
  2019-10-12  9:04     ` [PATCH 0/2] drm/imx: Adjustments for two functions Markus Elfring
  0 siblings, 2 replies; 15+ messages in thread
From: Navid Emamdoost @ 2019-10-07  4:26 UTC (permalink / raw)
  To: Markus Elfring
  Cc: kernel-janitors, Daniel Vetter, David Airlie, Shawn Guo,
	Sascha Hauer, Kangjie Lu, LKML, dri-devel, Navid Emamdoost,
	NXP Linux Team, Pengutronix Kernel Team, Stephen McCamant,
	Philipp Zabel, Fabio Estevam, linux-arm-kernel

Hi Markus,

I agree with you, kmemdup may fail so a null check seems necessary there.

On Sun, Oct 6, 2019 at 4:33 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> I have taken another look also at the implementation of the function “imx_pd_bind”.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/imx/parallel-display.c?id=43b815c6a8e7dbccb5b8bd9c4b099c24bc22d135#n197
> https://elixir.bootlin.com/linux/v5.4-rc1/source/drivers/gpu/drm/imx/parallel-display.c#L197
>
> Now I find an unchecked call of the function “kmemdup” suspicious.
> Will this detail trigger further software development considerations?
>
> Regards,
> Markus



-- 
Navid.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: drm/imx: Checking a kmemdup() call in imx_pd_bind()
  2019-10-07  4:26   ` Navid Emamdoost
@ 2019-10-07  7:44     ` Markus Elfring
  2019-10-12  9:04     ` [PATCH 0/2] drm/imx: Adjustments for two functions Markus Elfring
  1 sibling, 0 replies; 15+ messages in thread
From: Markus Elfring @ 2019-10-07  7:44 UTC (permalink / raw)
  To: Navid Emamdoost, Kangjie Lu, Stephen McCamant
  Cc: kernel, David Airlie, Shawn Guo, Sascha Hauer, kernel-janitors,
	LKML, dri-devel, linux-imx, Daniel Vetter, Philipp Zabel,
	Fabio Estevam, linux-arm-kernel, Navid Emamdoost

> I agree with you, kmemdup may fail so a null check seems necessary there.

Would this place (and similar ones) be pointed out for further considerations
also by the source code analysis tool which your software research group
seems to be developing?
https://github.com/umnsec/cheq/

Regards,
Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* [PATCH 0/2] drm/imx: Adjustments for two functions
  2019-10-07  4:26   ` Navid Emamdoost
  2019-10-07  7:44     ` Markus Elfring
@ 2019-10-12  9:04     ` Markus Elfring
  2019-10-12  9:07       ` [PATCH 1/2] drm/imx: Fix error handling for a kmemdup() call in imx_pd_bind() Markus Elfring
  2019-10-12  9:10       ` [PATCH 2/2] drm/imx: Fix error handling for a kmemdup() call in imx_ldb_panel_ddc() Markus Elfring
  1 sibling, 2 replies; 15+ messages in thread
From: Markus Elfring @ 2019-10-12  9:04 UTC (permalink / raw)
  To: dri-devel, kernel, linux-arm-kernel, linux-imx, Daniel Vetter,
	David Airlie, Fabio Estevam, Philipp Zabel, Sascha Hauer,
	Shawn Guo, Navid Emamdoost, Peter Senna Tschudin
  Cc: Rob Herring, kernel-janitors, Kangjie Lu, LKML, Thierry Reding,
	Stephen McCamant, Navid Emamdoost

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Oct 2019 10:45:45 +0200

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Fix error handling for a kmemdup() call in imx_pd_bind()
  Fix error handling for a kmemdup() call in imx_ldb_panel_ddc()

 drivers/gpu/drm/imx/imx-ldb.c          | 2 ++
 drivers/gpu/drm/imx/parallel-display.c | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

--
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* [PATCH 1/2] drm/imx: Fix error handling for a kmemdup() call in imx_pd_bind()
  2019-10-12  9:04     ` [PATCH 0/2] drm/imx: Adjustments for two functions Markus Elfring
@ 2019-10-12  9:07       ` Markus Elfring
  2019-10-12 19:16         ` Navid Emamdoost
  2019-10-12  9:10       ` [PATCH 2/2] drm/imx: Fix error handling for a kmemdup() call in imx_ldb_panel_ddc() Markus Elfring
  1 sibling, 1 reply; 15+ messages in thread
From: Markus Elfring @ 2019-10-12  9:07 UTC (permalink / raw)
  To: dri-devel, kernel, linux-arm-kernel, linux-imx, Daniel Vetter,
	David Airlie, Fabio Estevam, Philipp Zabel, Sascha Hauer,
	Shawn Guo, Navid Emamdoost, Peter Senna Tschudin
  Cc: Rob Herring, kernel-janitors, Kangjie Lu, LKML, Thierry Reding,
	Stephen McCamant, Navid Emamdoost

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Oct 2019 10:30:21 +0200

The return value from a call of the function “kmemdup” was not checked
in this function implementation. Thus add the corresponding error handling.

Fixes: 19022aaae677dfa171a719e9d1ff04823ce65a65 ("staging: drm/imx: Add parallel display support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/imx/parallel-display.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 35518e5de356..39c4798f56b6 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -210,8 +210,13 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 		return -ENOMEM;

 	edidp = of_get_property(np, "edid", &imxpd->edid_len);
-	if (edidp)
+	if (edidp) {
 		imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
+		if (!imxpd->edid) {
+			devm_kfree(dev, imxpd);
+			return -ENOMEM;
+		}
+	}

 	ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
 	if (!ret) {
--
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* [PATCH 2/2] drm/imx: Fix error handling for a kmemdup() call in imx_ldb_panel_ddc()
  2019-10-12  9:04     ` [PATCH 0/2] drm/imx: Adjustments for two functions Markus Elfring
  2019-10-12  9:07       ` [PATCH 1/2] drm/imx: Fix error handling for a kmemdup() call in imx_pd_bind() Markus Elfring
@ 2019-10-12  9:10       ` Markus Elfring
  1 sibling, 0 replies; 15+ messages in thread
From: Markus Elfring @ 2019-10-12  9:10 UTC (permalink / raw)
  To: dri-devel, kernel, linux-arm-kernel, linux-imx, Daniel Vetter,
	David Airlie, Fabio Estevam, Philipp Zabel, Sascha Hauer,
	Shawn Guo, Navid Emamdoost, Peter Senna Tschudin
  Cc: Rob Herring, kernel-janitors, Kangjie Lu, LKML, Thierry Reding,
	Stephen McCamant, Navid Emamdoost

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 12 Oct 2019 10:33:47 +0200

The return value from a call of the function “kmemdup” was not checked
in this function implementation. Thus add the corresponding error handling.

This issue was detected by using the Coccinelle software.

Fixes: dc80d7038883feca2abd08975165bc0d83c84762 ("drm/imx-ldb: Add support to drm-bridge")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/imx/imx-ldb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 208069faf183..801a2265dd11 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -569,6 +569,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
 			channel->edid = kmemdup(edidp,
 						channel->edid_len,
 						GFP_KERNEL);
+			if (!channel->edid)
+				return -ENOMEM;
 		} else if (!channel->panel) {
 			/* fallback to display-timings node */
 			ret = of_get_drm_display_mode(child,
--
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] drm/imx: fix memory leak in imx_pd_bind
  2019-10-05 15:02 ` Markus Elfring
@ 2019-10-12 11:54   ` Markus Elfring
  2019-10-12 19:22     ` Navid Emamdoost
  0 siblings, 1 reply; 15+ messages in thread
From: Markus Elfring @ 2019-10-12 11:54 UTC (permalink / raw)
  To: Navid Emamdoost, dri-devel, linux-arm-kernel, linux-imx, kernel
  Cc: kernel-janitors, Philipp Zabel, David Airlie, Shawn Guo,
	Sascha Hauer, Kangjie Lu, linux-kernel, Navid Emamdoost,
	Daniel Vetter, Stephen McCamant, Fabio Estevam

> +free_edid:
> + 	kfree(imxpd->edid);
> + 	return ret;

I have taken another look at this change idea.
Can the function call “devm_kfree(dev, imxpd)” become relevant
also at this place?

Would you like to combine it with the update suggestion
“Fix error handling for a kmemdup() call in imx_pd_bind()”?
https://lore.kernel.org/r/3fd6aa8b-2529-7ff5-3e19-05267101b2a4@web.de/
https://lore.kernel.org/patchwork/patch/1138912/
https://lkml.org/lkml/2019/10/12/87

Regards,
Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH 1/2] drm/imx: Fix error handling for a kmemdup() call in imx_pd_bind()
  2019-10-12  9:07       ` [PATCH 1/2] drm/imx: Fix error handling for a kmemdup() call in imx_pd_bind() Markus Elfring
@ 2019-10-12 19:16         ` Navid Emamdoost
  2019-10-12 19:24           ` Julia Lawall
  0 siblings, 1 reply; 15+ messages in thread
From: Navid Emamdoost @ 2019-10-12 19:16 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Thierry Reding, Rob Herring, kernel-janitors,
	Pengutronix Kernel Team, David Airlie, Shawn Guo, Sascha Hauer,
	Kangjie Lu, LKML, dri-devel, Navid Emamdoost,
	Peter Senna Tschudin, NXP Linux Team, Daniel Vetter,
	Stephen McCamant, Philipp Zabel, Fabio Estevam, linux-arm-kernel

On Sat, Oct 12, 2019 at 4:07 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 12 Oct 2019 10:30:21 +0200
>
> The return value from a call of the function “kmemdup” was not checked
> in this function implementation. Thus add the corresponding error handling.
>
> Fixes: 19022aaae677dfa171a719e9d1ff04823ce65a65 ("staging: drm/imx: Add parallel display support")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/imx/parallel-display.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index 35518e5de356..39c4798f56b6 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -210,8 +210,13 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>                 return -ENOMEM;
>
>         edidp = of_get_property(np, "edid", &imxpd->edid_len);
> -       if (edidp)
> +       if (edidp) {
>                 imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
> +               if (!imxpd->edid) {
> +                       devm_kfree(dev, imxpd);

You should not try to free imxpd here as it is a resource-managed
allocation via devm_kzalloc(). It means memory allocated with this
function is
 automatically freed on driver detach. So, this patch introduces a double-free.

> +                       return -ENOMEM;
> +               }
> +       }
>
>         ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
>         if (!ret) {
> --
> 2.23.0
>


-- 
Navid.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] drm/imx: fix memory leak in imx_pd_bind
  2019-10-12 11:54   ` Markus Elfring
@ 2019-10-12 19:22     ` Navid Emamdoost
  0 siblings, 0 replies; 15+ messages in thread
From: Navid Emamdoost @ 2019-10-12 19:22 UTC (permalink / raw)
  To: Markus Elfring
  Cc: kernel-janitors, Daniel Vetter, David Airlie, Shawn Guo,
	Sascha Hauer, Kangjie Lu, LKML, dri-devel, Navid Emamdoost,
	NXP Linux Team, Pengutronix Kernel Team, Stephen McCamant,
	Philipp Zabel, Fabio Estevam, linux-arm-kernel

No, that is not correct! You should not try to free imxpd here as it
is a resource-managed allocation via devm_kzalloc(). It means memory
allocated with this function is automatically freed on driver detach.
So, this patch introduces a double-free.

On Sat, Oct 12, 2019 at 6:54 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > +free_edid:
> > +     kfree(imxpd->edid);
> > +     return ret;
>
> I have taken another look at this change idea.
> Can the function call “devm_kfree(dev, imxpd)” become relevant
> also at this place?
>
> Would you like to combine it with the update suggestion
> “Fix error handling for a kmemdup() call in imx_pd_bind()”?
> https://lore.kernel.org/r/3fd6aa8b-2529-7ff5-3e19-05267101b2a4@web.de/
> https://lore.kernel.org/patchwork/patch/1138912/
> https://lkml.org/lkml/2019/10/12/87
>
> Regards,
> Markus



-- 
Navid.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH 1/2] drm/imx: Fix error handling for a kmemdup() call in imx_pd_bind()
  2019-10-12 19:16         ` Navid Emamdoost
@ 2019-10-12 19:24           ` Julia Lawall
  0 siblings, 0 replies; 15+ messages in thread
From: Julia Lawall @ 2019-10-12 19:24 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: Thierry Reding, Rob Herring, kernel-janitors,
	Peter Senna Tschudin, Pengutronix Kernel Team, David Airlie,
	Shawn Guo, Sascha Hauer, Kangjie Lu, LKML, dri-devel,
	Navid Emamdoost, Markus Elfring, NXP Linux Team, Daniel Vetter,
	Stephen McCamant, Philipp Zabel, Fabio Estevam, linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]



On Sat, 12 Oct 2019, Navid Emamdoost wrote:

> On Sat, Oct 12, 2019 at 4:07 AM Markus Elfring <Markus.Elfring@web.de> wrote:
> >
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sat, 12 Oct 2019 10:30:21 +0200
> >
> > The return value from a call of the function “kmemdup” was not checked
> > in this function implementation. Thus add the corresponding error handling.
> >
> > Fixes: 19022aaae677dfa171a719e9d1ff04823ce65a65 ("staging: drm/imx: Add parallel display support")
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/gpu/drm/imx/parallel-display.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> > index 35518e5de356..39c4798f56b6 100644
> > --- a/drivers/gpu/drm/imx/parallel-display.c
> > +++ b/drivers/gpu/drm/imx/parallel-display.c
> > @@ -210,8 +210,13 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> >                 return -ENOMEM;
> >
> >         edidp = of_get_property(np, "edid", &imxpd->edid_len);
> > -       if (edidp)
> > +       if (edidp) {
> >                 imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
> > +               if (!imxpd->edid) {
> > +                       devm_kfree(dev, imxpd);
>
> You should not try to free imxpd here as it is a resource-managed
> allocation via devm_kzalloc(). It means memory allocated with this
> function is
>  automatically freed on driver detach. So, this patch introduces a double-free.

No, it's not double freed since the proposed code frees it with a devm
function, removing it from the list of things to free later.  One can
wonder why the free has to be made apparent, though.

julia

>
> > +                       return -ENOMEM;
> > +               }
> > +       }
> >
> >         ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
> >         if (!ret) {
> > --
> > 2.23.0
> >
>
>
> --
> Navid.
>

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] drm/imx: fix memory leak in imx_pd_bind
  2019-10-04 19:09 [PATCH] drm/imx: fix memory leak in imx_pd_bind Navid Emamdoost
  2019-10-05 15:02 ` Markus Elfring
  2019-10-06  9:33 ` drm/imx: Checking a kmemdup() call in imx_pd_bind() Markus Elfring
@ 2019-11-21 18:31 ` Navid Emamdoost
  2019-11-22  7:22   ` Marco Felsch
  2 siblings, 1 reply; 15+ messages in thread
From: Navid Emamdoost @ 2019-11-21 18:31 UTC (permalink / raw)
  To: Philipp Zabel, David Airlie, Daniel Vetter, Sascha Hauer,
	Shawn Guo, Fabio Estevam
  Cc: LKML, dri-devel, Navid Emamdoost, NXP Linux Team,
	Pengutronix Kernel Team, linux-arm-kernel

On Fri, Oct 4, 2019 at 2:09 PM Navid Emamdoost
<navid.emamdoost@gmail.com> wrote:
>
> In imx_pd_bind, the duplicated memory for imxpd->edid via kmemdup should
> be released in drm_of_find_panel_or_bridge or imx_pd_register fail.
>
> Fixes: ebc944613567 ("drm: convert drivers to use drm_of_find_panel_or_bridge")
> Fixes: 19022aaae677 ("staging: drm/imx: Add parallel display support")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---

Would you please review this patch?

Thanks,

>  drivers/gpu/drm/imx/parallel-display.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index e7ce17503ae1..9522d2cb0ad5 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -227,14 +227,18 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>
>         /* port@1 is the output port */
>         ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
> -       if (ret && ret != -ENODEV)
> +       if (ret && ret != -ENODEV) {
> +               kfree(imxpd->edid);
>                 return ret;
> +       }
>
>         imxpd->dev = dev;
>
>         ret = imx_pd_register(drm, imxpd);
> -       if (ret)
> +       if (ret) {
> +               kfree(imxpd->edid);
>                 return ret;
> +       }
>
>         dev_set_drvdata(dev, imxpd);
>
> --
> 2.17.1
>


-- 
Navid.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] drm/imx: fix memory leak in imx_pd_bind
  2019-11-21 18:31 ` [PATCH] drm/imx: fix memory leak in imx_pd_bind Navid Emamdoost
@ 2019-11-22  7:22   ` Marco Felsch
  2019-11-22 17:43     ` Navid Emamdoost
  0 siblings, 1 reply; 15+ messages in thread
From: Marco Felsch @ 2019-11-22  7:22 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: Philipp Zabel, David Airlie, Shawn Guo, Sascha Hauer, LKML,
	dri-devel, Navid Emamdoost, NXP Linux Team, Daniel Vetter,
	Fabio Estevam, Pengutronix Kernel Team, linux-arm-kernel

Hi Navid,

On 19-11-21 12:31, Navid Emamdoost wrote:
> On Fri, Oct 4, 2019 at 2:09 PM Navid Emamdoost
> <navid.emamdoost@gmail.com> wrote:
> >
> > In imx_pd_bind, the duplicated memory for imxpd->edid via kmemdup should
> > be released in drm_of_find_panel_or_bridge or imx_pd_register fail.
> >
> > Fixes: ebc944613567 ("drm: convert drivers to use drm_of_find_panel_or_bridge")
> > Fixes: 19022aaae677 ("staging: drm/imx: Add parallel display support")
> > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> > ---
> 
> Would you please review this patch?
> 
> Thanks,

I currently work on the drm/imx driver(s) to avoid errors like [1].
Hopefully I have a working version till next week. There I fixed this
issue by using the devm_kmemdup() and dropped the explicit kfree()
within unbind().

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

Regards,
  Marco

> 
> >  drivers/gpu/drm/imx/parallel-display.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> > index e7ce17503ae1..9522d2cb0ad5 100644
> > --- a/drivers/gpu/drm/imx/parallel-display.c
> > +++ b/drivers/gpu/drm/imx/parallel-display.c
> > @@ -227,14 +227,18 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> >
> >         /* port@1 is the output port */
> >         ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
> > -       if (ret && ret != -ENODEV)
> > +       if (ret && ret != -ENODEV) {
> > +               kfree(imxpd->edid);
> >                 return ret;
> > +       }
> >
> >         imxpd->dev = dev;
> >
> >         ret = imx_pd_register(drm, imxpd);
> > -       if (ret)
> > +       if (ret) {
> > +               kfree(imxpd->edid);
> >                 return ret;
> > +       }
> >
> >         dev_set_drvdata(dev, imxpd);
> >
> > --
> > 2.17.1
> >
> 
> 
> -- 
> Navid.
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] drm/imx: fix memory leak in imx_pd_bind
  2019-11-22  7:22   ` Marco Felsch
@ 2019-11-22 17:43     ` Navid Emamdoost
  0 siblings, 0 replies; 15+ messages in thread
From: Navid Emamdoost @ 2019-11-22 17:43 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Philipp Zabel, David Airlie, Shawn Guo, Sascha Hauer, LKML,
	dri-devel, Navid Emamdoost, NXP Linux Team, Daniel Vetter,
	Fabio Estevam, Pengutronix Kernel Team, linux-arm-kernel

Thanks for the update.

On Fri, Nov 22, 2019 at 1:22 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi Navid,
>
> On 19-11-21 12:31, Navid Emamdoost wrote:
> > On Fri, Oct 4, 2019 at 2:09 PM Navid Emamdoost
> > <navid.emamdoost@gmail.com> wrote:
> > >
> > > In imx_pd_bind, the duplicated memory for imxpd->edid via kmemdup should
> > > be released in drm_of_find_panel_or_bridge or imx_pd_register fail.
> > >
> > > Fixes: ebc944613567 ("drm: convert drivers to use drm_of_find_panel_or_bridge")
> > > Fixes: 19022aaae677 ("staging: drm/imx: Add parallel display support")
> > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> > > ---
> >
> > Would you please review this patch?
> >
> > Thanks,
>
> I currently work on the drm/imx driver(s) to avoid errors like [1].
> Hopefully I have a working version till next week. There I fixed this
> issue by using the devm_kmemdup() and dropped the explicit kfree()
> within unbind().
>
> [1] https://www.spinics.net/lists/dri-devel/msg189388.html
>
> Regards,
>   Marco
>
> >
> > >  drivers/gpu/drm/imx/parallel-display.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> > > index e7ce17503ae1..9522d2cb0ad5 100644
> > > --- a/drivers/gpu/drm/imx/parallel-display.c
> > > +++ b/drivers/gpu/drm/imx/parallel-display.c
> > > @@ -227,14 +227,18 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> > >
> > >         /* port@1 is the output port */
> > >         ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
> > > -       if (ret && ret != -ENODEV)
> > > +       if (ret && ret != -ENODEV) {
> > > +               kfree(imxpd->edid);
> > >                 return ret;
> > > +       }
> > >
> > >         imxpd->dev = dev;
> > >
> > >         ret = imx_pd_register(drm, imxpd);
> > > -       if (ret)
> > > +       if (ret) {
> > > +               kfree(imxpd->edid);
> > >                 return ret;
> > > +       }
> > >
> > >         dev_set_drvdata(dev, imxpd);
> > >
> > > --
> > > 2.17.1
> > >
> >
> >
> > --
> > Navid.
> >
> >
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



-- 
Navid.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-04 19:09 [PATCH] drm/imx: fix memory leak in imx_pd_bind Navid Emamdoost
2019-10-05 15:02 ` Markus Elfring
2019-10-12 11:54   ` Markus Elfring
2019-10-12 19:22     ` Navid Emamdoost
2019-10-06  9:33 ` drm/imx: Checking a kmemdup() call in imx_pd_bind() Markus Elfring
2019-10-07  4:26   ` Navid Emamdoost
2019-10-07  7:44     ` Markus Elfring
2019-10-12  9:04     ` [PATCH 0/2] drm/imx: Adjustments for two functions Markus Elfring
2019-10-12  9:07       ` [PATCH 1/2] drm/imx: Fix error handling for a kmemdup() call in imx_pd_bind() Markus Elfring
2019-10-12 19:16         ` Navid Emamdoost
2019-10-12 19:24           ` Julia Lawall
2019-10-12  9:10       ` [PATCH 2/2] drm/imx: Fix error handling for a kmemdup() call in imx_ldb_panel_ddc() Markus Elfring
2019-11-21 18:31 ` [PATCH] drm/imx: fix memory leak in imx_pd_bind Navid Emamdoost
2019-11-22  7:22   ` Marco Felsch
2019-11-22 17:43     ` Navid Emamdoost

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

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.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-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


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