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
> 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
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
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
> 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
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
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
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
> +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
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
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
[-- 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
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
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
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