| Message ID | 20181016160923.2042-1-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "drm/imx: don't destroy mode objects manually on driver unbind" | expand |
On Tue, Oct 16, 2018 at 06:09:23PM +0200, Stefan Agner wrote: > This reverts commit 8e3b16e2117409625b89807de3912ff773aea354. > > Using the component framework requires all components to undo in > ->unbind what ->bind does. Unfortunately that particular commit > broke this rule. In particular, this is an issue if a single > component during probe fails. In that case, component_bind_all() > calls unbind on already succussfully bound components, and then > frees memory allocated using devm. If one of those components > registered e.g. an encoder with the framework then this leads to > use after free situations. > > Revert the commit to ensure that all components properly undo > what ->bind does. > > Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk> > Signed-off-by: Stefan Agner <stefan@agner.ch> Adding Benjamin, who just made the same mistake I think (and I reviewed it ... oh well). -Daniel > --- > drivers/gpu/drm/imx/imx-drm-core.c | 4 ++-- > drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++ > drivers/gpu/drm/imx/imx-tve.c | 3 +++ > drivers/gpu/drm/imx/parallel-display.c | 3 +++ > 4 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c > index 5ea0c82f9957..caa6061a98ba 100644 > --- a/drivers/gpu/drm/imx/imx-drm-core.c > +++ b/drivers/gpu/drm/imx/imx-drm-core.c > @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev) > > drm_fb_cma_fbdev_fini(drm); > > - drm_mode_config_cleanup(drm); > - > component_unbind_all(drm->dev, drm); > dev_set_drvdata(dev, NULL); > > + drm_mode_config_cleanup(drm); > + > drm_dev_put(drm); > } > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c > index 3bd0f8a18e74..592aabc4a262 100644 > --- a/drivers/gpu/drm/imx/imx-ldb.c > +++ b/drivers/gpu/drm/imx/imx-ldb.c > @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master, > if (channel->panel) > drm_panel_detach(channel->panel); > > + if (!channel->connector.funcs) > + continue; > + > + channel->connector.funcs->destroy(&channel->connector); > + channel->encoder.funcs->destroy(&channel->encoder); > + > kfree(channel->edid); > i2c_put_adapter(channel->ddc); > } > diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c > index cffd3310240e..8d6e89ce1edb 100644 > --- a/drivers/gpu/drm/imx/imx-tve.c > +++ b/drivers/gpu/drm/imx/imx-tve.c > @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master, > { > struct imx_tve *tve = dev_get_drvdata(dev); > > + tve->connector.funcs->destroy(&tve->connector); > + tve->encoder.funcs->destroy(&tve->encoder); > + > if (!IS_ERR(tve->dac_reg)) > regulator_disable(tve->dac_reg); > } > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c > index aefd04e18f93..6f11bffcde37 100644 > --- a/drivers/gpu/drm/imx/parallel-display.c > +++ b/drivers/gpu/drm/imx/parallel-display.c > @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master, > if (imxpd->panel) > drm_panel_detach(imxpd->panel); > > + imxpd->encoder.funcs->destroy(&imxpd->encoder); > + imxpd->connector.funcs->destroy(&imxpd->connector); > + > kfree(imxpd->edid); > } > > -- > 2.19.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am Dienstag, den 16.10.2018, 18:09 +0200 schrieb Stefan Agner: > This reverts commit 8e3b16e2117409625b89807de3912ff773aea354. > > Using the component framework requires all components to undo in > ->unbind what ->bind does. Unfortunately that particular commit > broke this rule. In particular, this is an issue if a single > component during probe fails. In that case, component_bind_all() > calls unbind on already succussfully bound components, and then > frees memory allocated using devm. If one of those components > registered e.g. an encoder with the framework then this leads to > use after free situations. > > Revert the commit to ensure that all components properly undo > what ->bind does. I agree with this patch in general, but I think I remember why I changed this in the first place: dw_hdmi_imx_unbind does not destroy the encoder that has been registered in dw_hdmi_imx_bind. 8e3b16e21174 was an (apparently wrong) attempt to fix such issues once and for all. By reverting this commit we go back to leaking the HDMI encoder, so I think this patch should include a fix to destroy the encoder on unbind. Regards, Lucas > Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html > > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk> > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > drivers/gpu/drm/imx/imx-drm-core.c | 4 ++-- > drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++ > drivers/gpu/drm/imx/imx-tve.c | 3 +++ > drivers/gpu/drm/imx/parallel-display.c | 3 +++ > 4 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c > index 5ea0c82f9957..caa6061a98ba 100644 > --- a/drivers/gpu/drm/imx/imx-drm-core.c > +++ b/drivers/gpu/drm/imx/imx-drm-core.c > @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev) > > > drm_fb_cma_fbdev_fini(drm); > > > - drm_mode_config_cleanup(drm); > - > > component_unbind_all(drm->dev, drm); > > dev_set_drvdata(dev, NULL); > > > + drm_mode_config_cleanup(drm); > + > > drm_dev_put(drm); > } > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c > index 3bd0f8a18e74..592aabc4a262 100644 > --- a/drivers/gpu/drm/imx/imx-ldb.c > +++ b/drivers/gpu/drm/imx/imx-ldb.c > @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master, > > if (channel->panel) > > drm_panel_detach(channel->panel); > > > + if (!channel->connector.funcs) > > + continue; > + > > + channel->connector.funcs->destroy(&channel->connector); > > + channel->encoder.funcs->destroy(&channel->encoder); > + > > kfree(channel->edid); > > i2c_put_adapter(channel->ddc); > > } > diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c > index cffd3310240e..8d6e89ce1edb 100644 > --- a/drivers/gpu/drm/imx/imx-tve.c > +++ b/drivers/gpu/drm/imx/imx-tve.c > @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master, > { > > struct imx_tve *tve = dev_get_drvdata(dev); > > > + tve->connector.funcs->destroy(&tve->connector); > > + tve->encoder.funcs->destroy(&tve->encoder); > + > > if (!IS_ERR(tve->dac_reg)) > > regulator_disable(tve->dac_reg); > } > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c > index aefd04e18f93..6f11bffcde37 100644 > --- a/drivers/gpu/drm/imx/parallel-display.c > +++ b/drivers/gpu/drm/imx/parallel-display.c > @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master, > > if (imxpd->panel) > > drm_panel_detach(imxpd->panel); > > > + imxpd->encoder.funcs->destroy(&imxpd->encoder); > > + imxpd->connector.funcs->destroy(&imxpd->connector); > + > > kfree(imxpd->edid); > } >
On 16.10.2018 18:51, Lucas Stach wrote: > Am Dienstag, den 16.10.2018, 18:09 +0200 schrieb Stefan Agner: >> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354. >> >> Using the component framework requires all components to undo in >> ->unbind what ->bind does. Unfortunately that particular commit >> broke this rule. In particular, this is an issue if a single >> component during probe fails. In that case, component_bind_all() >> calls unbind on already succussfully bound components, and then >> frees memory allocated using devm. If one of those components >> registered e.g. an encoder with the framework then this leads to >> use after free situations. >> >> Revert the commit to ensure that all components properly undo >> what ->bind does. > > I agree with this patch in general, but I think I remember why I > changed this in the first place: dw_hdmi_imx_unbind does not destroy > the encoder that has been registered in dw_hdmi_imx_bind. Good point, yeah that is missing. > > 8e3b16e21174 was an (apparently wrong) attempt to fix such issues once > and for all. By reverting this commit we go back to leaking the HDMI > encoder, so I think this patch should include a fix to destroy the > encoder on unbind. Makes sense, will add that to the same commit and send a v2. -- Stefan > > Regards, > Lucas > >> Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html >> > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk> >> > Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> drivers/gpu/drm/imx/imx-drm-core.c | 4 ++-- >> drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++ >> drivers/gpu/drm/imx/imx-tve.c | 3 +++ >> drivers/gpu/drm/imx/parallel-display.c | 3 +++ >> 4 files changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c >> index 5ea0c82f9957..caa6061a98ba 100644 >> --- a/drivers/gpu/drm/imx/imx-drm-core.c >> +++ b/drivers/gpu/drm/imx/imx-drm-core.c >> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev) >> >> > drm_fb_cma_fbdev_fini(drm); >> >> > - drm_mode_config_cleanup(drm); >> - >> > component_unbind_all(drm->dev, drm); >> > dev_set_drvdata(dev, NULL); >> >> > + drm_mode_config_cleanup(drm); >> + >> > drm_dev_put(drm); >> } >> >> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c >> index 3bd0f8a18e74..592aabc4a262 100644 >> --- a/drivers/gpu/drm/imx/imx-ldb.c >> +++ b/drivers/gpu/drm/imx/imx-ldb.c >> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master, >> > if (channel->panel) >> > drm_panel_detach(channel->panel); >> >> > + if (!channel->connector.funcs) >> > + continue; >> + >> > + channel->connector.funcs->destroy(&channel->connector); >> > + channel->encoder.funcs->destroy(&channel->encoder); >> + >> > kfree(channel->edid); >> > i2c_put_adapter(channel->ddc); >> > } >> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c >> index cffd3310240e..8d6e89ce1edb 100644 >> --- a/drivers/gpu/drm/imx/imx-tve.c >> +++ b/drivers/gpu/drm/imx/imx-tve.c >> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master, >> { >> > struct imx_tve *tve = dev_get_drvdata(dev); >> >> > + tve->connector.funcs->destroy(&tve->connector); >> > + tve->encoder.funcs->destroy(&tve->encoder); >> + >> > if (!IS_ERR(tve->dac_reg)) >> > regulator_disable(tve->dac_reg); >> } >> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c >> index aefd04e18f93..6f11bffcde37 100644 >> --- a/drivers/gpu/drm/imx/parallel-display.c >> +++ b/drivers/gpu/drm/imx/parallel-display.c >> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master, >> > if (imxpd->panel) >> > drm_panel_detach(imxpd->panel); >> >> > + imxpd->encoder.funcs->destroy(&imxpd->encoder); >> > + imxpd->connector.funcs->destroy(&imxpd->connector); >> + >> > kfree(imxpd->edid); >> } >>
On 16.10.2018 18:09, Stefan Agner wrote: > This reverts commit 8e3b16e2117409625b89807de3912ff773aea354. > > Using the component framework requires all components to undo in > ->unbind what ->bind does. Unfortunately that particular commit > broke this rule. In particular, this is an issue if a single > component during probe fails. In that case, component_bind_all() > calls unbind on already succussfully bound components, and then > frees memory allocated using devm. If one of those components > registered e.g. an encoder with the framework then this leads to > use after free situations. > > Revert the commit to ensure that all components properly undo > what ->bind does. After Lucas comment mentioning HDMI unbind is not proper I looked through all the unbind again. The other unbind functions need some fixing too. I did not bother checking whether those were always broken or just because things changed (the commit this is reverting was in 2016).... Here is what I found: > > Link: > https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk> > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > drivers/gpu/drm/imx/imx-drm-core.c | 4 ++-- > drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++ > drivers/gpu/drm/imx/imx-tve.c | 3 +++ > drivers/gpu/drm/imx/parallel-display.c | 3 +++ > 4 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c > b/drivers/gpu/drm/imx/imx-drm-core.c > index 5ea0c82f9957..caa6061a98ba 100644 > --- a/drivers/gpu/drm/imx/imx-drm-core.c > +++ b/drivers/gpu/drm/imx/imx-drm-core.c > @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev) > > drm_fb_cma_fbdev_fini(drm); > > - drm_mode_config_cleanup(drm); > - > component_unbind_all(drm->dev, drm); > dev_set_drvdata(dev, NULL); > > + drm_mode_config_cleanup(drm); > + > drm_dev_put(drm); > } > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c > index 3bd0f8a18e74..592aabc4a262 100644 > --- a/drivers/gpu/drm/imx/imx-ldb.c > +++ b/drivers/gpu/drm/imx/imx-ldb.c > @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, > struct device *master, > if (channel->panel) > drm_panel_detach(channel->panel); > > + if (!channel->connector.funcs) > + continue; > + > + channel->connector.funcs->destroy(&channel->connector); > + channel->encoder.funcs->destroy(&channel->encoder); There can be an encoder and bridge, or an encoder and connector. All of them should be properly cleaned up. So I guess this should look like this: if (channel->panel) drm_panel_detach(channel->panel); if (channel->bridge) drm_bridge_detach(channel->bridge); if (channel->connector.funcs) channel->connector.funcs->destroy(&channel->connector); if (channel->encoder.funcs) channel->encoder.funcs->destroy(&channel->encoder); kfree(channel->edid); i2c_put_adapter(channel->ddc); The last two functions following are only strictly necessary when connector is initialized. But its safe to call them with null, so I would just call them always. > + > kfree(channel->edid); > i2c_put_adapter(channel->ddc); > } > diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c > index cffd3310240e..8d6e89ce1edb 100644 > --- a/drivers/gpu/drm/imx/imx-tve.c > +++ b/drivers/gpu/drm/imx/imx-tve.c > @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, > struct device *master, > { > struct imx_tve *tve = dev_get_drvdata(dev); > > + tve->connector.funcs->destroy(&tve->connector); > + tve->encoder.funcs->destroy(&tve->encoder); > + Cleanup of tve->ddc missing. > if (!IS_ERR(tve->dac_reg)) > regulator_disable(tve->dac_reg); > } > diff --git a/drivers/gpu/drm/imx/parallel-display.c > b/drivers/gpu/drm/imx/parallel-display.c > index aefd04e18f93..6f11bffcde37 100644 > --- a/drivers/gpu/drm/imx/parallel-display.c > +++ b/drivers/gpu/drm/imx/parallel-display.c > @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, > struct device *master, > if (imxpd->panel) > drm_panel_detach(imxpd->panel); > And in this case a bridge detach is missing. Will send a v2 with that addressed. -- Stefan > + imxpd->encoder.funcs->destroy(&imxpd->encoder); > + imxpd->connector.funcs->destroy(&imxpd->connector); > + > kfree(imxpd->edid); > }
On 17.10.2018 13:25, Stefan Agner wrote: > On 16.10.2018 18:09, Stefan Agner wrote: >> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354. >> >> Using the component framework requires all components to undo in >> ->unbind what ->bind does. Unfortunately that particular commit >> broke this rule. In particular, this is an issue if a single >> component during probe fails. In that case, component_bind_all() >> calls unbind on already succussfully bound components, and then >> frees memory allocated using devm. If one of those components >> registered e.g. an encoder with the framework then this leads to >> use after free situations. >> >> Revert the commit to ensure that all components properly undo >> what ->bind does. > > After Lucas comment mentioning HDMI unbind is not proper I looked > through all the unbind again. The other unbind functions need some > fixing too. I did not bother checking whether those were always broken > or just because things changed (the commit this is reverting was in > 2016).... Here is what I found: > >> >> Link: >> https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html >> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk> >> Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> drivers/gpu/drm/imx/imx-drm-core.c | 4 ++-- >> drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++ >> drivers/gpu/drm/imx/imx-tve.c | 3 +++ >> drivers/gpu/drm/imx/parallel-display.c | 3 +++ >> 4 files changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c >> b/drivers/gpu/drm/imx/imx-drm-core.c >> index 5ea0c82f9957..caa6061a98ba 100644 >> --- a/drivers/gpu/drm/imx/imx-drm-core.c >> +++ b/drivers/gpu/drm/imx/imx-drm-core.c >> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev) >> >> drm_fb_cma_fbdev_fini(drm); >> >> - drm_mode_config_cleanup(drm); >> - >> component_unbind_all(drm->dev, drm); >> dev_set_drvdata(dev, NULL); >> >> + drm_mode_config_cleanup(drm); >> + >> drm_dev_put(drm); >> } >> >> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c >> index 3bd0f8a18e74..592aabc4a262 100644 >> --- a/drivers/gpu/drm/imx/imx-ldb.c >> +++ b/drivers/gpu/drm/imx/imx-ldb.c >> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, >> struct device *master, >> if (channel->panel) >> drm_panel_detach(channel->panel); >> >> + if (!channel->connector.funcs) >> + continue; >> + >> + channel->connector.funcs->destroy(&channel->connector); >> + channel->encoder.funcs->destroy(&channel->encoder); > > There can be an encoder and bridge, or an encoder and connector. All of > them should be properly cleaned up. > > So I guess this should look like this: > > if (channel->panel) > drm_panel_detach(channel->panel); > > if (channel->bridge) > drm_bridge_detach(channel->bridge); Actually, when a bridge is attached to an encoder, which is the case when unbind is getting called, then encoder cleanup takes care of drm_bridge_detach. > > if (channel->connector.funcs) > channel->connector.funcs->destroy(&channel->connector); > > if (channel->encoder.funcs) > channel->encoder.funcs->destroy(&channel->encoder); > > kfree(channel->edid); > i2c_put_adapter(channel->ddc); > > The last two functions following are only strictly necessary when > connector is initialized. But its safe to call them with null, so I > would just call them always. > > >> + >> kfree(channel->edid); >> i2c_put_adapter(channel->ddc); >> } >> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c >> index cffd3310240e..8d6e89ce1edb 100644 >> --- a/drivers/gpu/drm/imx/imx-tve.c >> +++ b/drivers/gpu/drm/imx/imx-tve.c >> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, >> struct device *master, >> { >> struct imx_tve *tve = dev_get_drvdata(dev); >> >> + tve->connector.funcs->destroy(&tve->connector); >> + tve->encoder.funcs->destroy(&tve->encoder); >> + > > Cleanup of tve->ddc missing. > >> if (!IS_ERR(tve->dac_reg)) >> regulator_disable(tve->dac_reg); >> } >> diff --git a/drivers/gpu/drm/imx/parallel-display.c >> b/drivers/gpu/drm/imx/parallel-display.c >> index aefd04e18f93..6f11bffcde37 100644 >> --- a/drivers/gpu/drm/imx/parallel-display.c >> +++ b/drivers/gpu/drm/imx/parallel-display.c >> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, >> struct device *master, >> if (imxpd->panel) >> drm_panel_detach(imxpd->panel); >> > > And in this case a bridge detach is missing. Same here. > > Will send a v2 with that addressed. Still valid. -- Stefan > > -- > Stefan > >> + imxpd->encoder.funcs->destroy(&imxpd->encoder); >> + imxpd->connector.funcs->destroy(&imxpd->connector); >> + >> kfree(imxpd->edid); >> }
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 5ea0c82f9957..caa6061a98ba 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev) drm_fb_cma_fbdev_fini(drm); - drm_mode_config_cleanup(drm); - component_unbind_all(drm->dev, drm); dev_set_drvdata(dev, NULL); + drm_mode_config_cleanup(drm); + drm_dev_put(drm); } diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index 3bd0f8a18e74..592aabc4a262 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master, if (channel->panel) drm_panel_detach(channel->panel); + if (!channel->connector.funcs) + continue; + + channel->connector.funcs->destroy(&channel->connector); + channel->encoder.funcs->destroy(&channel->encoder); + kfree(channel->edid); i2c_put_adapter(channel->ddc); } diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c index cffd3310240e..8d6e89ce1edb 100644 --- a/drivers/gpu/drm/imx/imx-tve.c +++ b/drivers/gpu/drm/imx/imx-tve.c @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master, { struct imx_tve *tve = dev_get_drvdata(dev); + tve->connector.funcs->destroy(&tve->connector); + tve->encoder.funcs->destroy(&tve->encoder); + if (!IS_ERR(tve->dac_reg)) regulator_disable(tve->dac_reg); } diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index aefd04e18f93..6f11bffcde37 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master, if (imxpd->panel) drm_panel_detach(imxpd->panel); + imxpd->encoder.funcs->destroy(&imxpd->encoder); + imxpd->connector.funcs->destroy(&imxpd->connector); + kfree(imxpd->edid); }
This reverts commit 8e3b16e2117409625b89807de3912ff773aea354. Using the component framework requires all components to undo in ->unbind what ->bind does. Unfortunately that particular commit broke this rule. In particular, this is an issue if a single component during probe fails. In that case, component_bind_all() calls unbind on already succussfully bound components, and then frees memory allocated using devm. If one of those components registered e.g. an encoder with the framework then this leads to use after free situations. Revert the commit to ensure that all components properly undo what ->bind does. Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html Suggested-by: Russell King <rmk+kernel@armlinux.org.uk> Signed-off-by: Stefan Agner <stefan@agner.ch> --- drivers/gpu/drm/imx/imx-drm-core.c | 4 ++-- drivers/gpu/drm/imx/imx-ldb.c | 6 ++++++ drivers/gpu/drm/imx/imx-tve.c | 3 +++ drivers/gpu/drm/imx/parallel-display.c | 3 +++ 4 files changed, 14 insertions(+), 2 deletions(-)