drm/imx: fix use after free

[ Upstream commit ba807c94f67fd64b3051199810d9e4dd209fdc00 ]

Component driver structures allocated with devm_kmalloc() in bind() are
freed automatically after unbind(). Since the contained drm structures
are accessed afterwards in drm_mode_config_cleanup(), move the
allocation into probe() to extend the driver structure's lifetime to the
lifetime of the device. This should eventually be changed to use drm
resource managed allocations with lifetime of the drm device.

We also need to ensure that all componets are available during the
unbind() so we need to call component_unbind_all() before we free
non-devres resources like planes.

Note this patch fixes the the use after free bug but introduces a
possible boot loop issue. The issue is triggered if the HDMI support is
enabled and a component driver always return -EPROBE_DEFER, see
discussion [1] for more details.

[1] https://lkml.org/lkml/2020/3/24/1467

Fixes: 17b5001b5143 ("imx-drm: convert to componentised device support")
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
[m.felsch@pengutronix: fix imx_tve_probe()]
[m.felsch@pengutronix: resort component_unbind_all())
[m.felsch@pengutronix: adapt commit message]
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index f22cfbf9353ede21121809335751130a9777217a..2e12a4a3bfa11b9b7f2e61533d281d5b8ad31f4d 100644 (file)
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -212,9 +212,8 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
        if (!pdev->dev.of_node)
                return -ENODEV;
 
-       hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
-       if (!hdmi)
-               return -ENOMEM;
+       hdmi = dev_get_drvdata(dev);
+       memset(hdmi, 0, sizeof(*hdmi));
 
        match = of_match_node(dw_hdmi_imx_dt_ids, pdev->dev.of_node);
        plat_data = match->data;
@@ -239,8 +238,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
        drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
                         DRM_MODE_ENCODER_TMDS, NULL);
 
-       platform_set_drvdata(pdev, hdmi);
-
        hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
 
        /*
@@ -270,6 +267,14 @@ static const struct component_ops dw_hdmi_imx_ops = {
 
 static int dw_hdmi_imx_probe(struct platform_device *pdev)
 {
+       struct imx_hdmi *hdmi;
+
+       hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+       if (!hdmi)
+               return -ENOMEM;
+
+       platform_set_drvdata(pdev, hdmi);
+
        return component_add(&pdev->dev, &dw_hdmi_imx_ops);
 }
 

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index da87c70e413b4d6d821b991621563c3edf618fc0..881c36d0f16bb822411016ab50967937d1b6bdea 100644 (file)
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -281,9 +281,10 @@ static void imx_drm_unbind(struct device *dev)
 
        drm_kms_helper_poll_fini(drm);
 
+       component_unbind_all(drm->dev, drm);
+
        drm_mode_config_cleanup(drm);
 
-       component_unbind_all(drm->dev, drm);
        dev_set_drvdata(dev, NULL);
 
        drm_dev_put(drm);

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 4da22a94790cfca5059de6ab720067154e3cd9ba..8e209117b049ab443387f9c277edf06e781b54f2 100644 (file)
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -594,9 +594,8 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
        int ret;
        int i;
 
-       imx_ldb = devm_kzalloc(dev, sizeof(*imx_ldb), GFP_KERNEL);
-       if (!imx_ldb)
-               return -ENOMEM;
+       imx_ldb = dev_get_drvdata(dev);
+       memset(imx_ldb, 0, sizeof(*imx_ldb));
 
        imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
        if (IS_ERR(imx_ldb->regmap)) {
@@ -704,8 +703,6 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
                }
        }
 
-       dev_set_drvdata(dev, imx_ldb);
-
        return 0;
 
 free_child:
@@ -737,6 +734,14 @@ static const struct component_ops imx_ldb_ops = {
 
 static int imx_ldb_probe(struct platform_device *pdev)
 {
+       struct imx_ldb *imx_ldb;
+
+       imx_ldb = devm_kzalloc(&pdev->dev, sizeof(*imx_ldb), GFP_KERNEL);
+       if (!imx_ldb)
+               return -ENOMEM;
+
+       platform_set_drvdata(pdev, imx_ldb);
+
        return component_add(&pdev->dev, &imx_ldb_ops);
 }
 

diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 5bbfaa2cd0f47af15f0594f5001857ed5440dcbb..9fd4b464e829cfa867387e309ceb62e68e93ffc9 100644 (file)
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -546,9 +546,8 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
        int irq;
        int ret;
 
-       tve = devm_kzalloc(dev, sizeof(*tve), GFP_KERNEL);
-       if (!tve)
-               return -ENOMEM;
+       tve = dev_get_drvdata(dev);
+       memset(tve, 0, sizeof(*tve));
 
        tve->dev = dev;
        spin_lock_init(&tve->lock);
@@ -659,8 +658,6 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
        if (ret)
                return ret;
 
-       dev_set_drvdata(dev, tve);
-
        return 0;
 }
 
@@ -680,6 +677,14 @@ static const struct component_ops imx_tve_ops = {
 
 static int imx_tve_probe(struct platform_device *pdev)
 {
+       struct imx_tve *tve;
+
+       tve = devm_kzalloc(&pdev->dev, sizeof(*tve), GFP_KERNEL);
+       if (!tve)
+               return -ENOMEM;
+
+       platform_set_drvdata(pdev, tve);
+
        return component_add(&pdev->dev, &imx_tve_ops);
 }
 

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 63c0284f8b3c07f01d74c552fdb3829ad81fad7e..2256c9789fc2c4a9e55e7bb6fd67c93d7116faa9 100644 (file)
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -438,21 +438,13 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
        struct ipu_client_platformdata *pdata = dev->platform_data;
        struct drm_device *drm = data;
        struct ipu_crtc *ipu_crtc;
-       int ret;
 
-       ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
-       if (!ipu_crtc)
-               return -ENOMEM;
+       ipu_crtc = dev_get_drvdata(dev);
+       memset(ipu_crtc, 0, sizeof(*ipu_crtc));
 
        ipu_crtc->dev = dev;
 
-       ret = ipu_crtc_init(ipu_crtc, pdata, drm);
-       if (ret)
-               return ret;
-
-       dev_set_drvdata(dev, ipu_crtc);
-
-       return 0;
+       return ipu_crtc_init(ipu_crtc, pdata, drm);
 }
 
 static void ipu_drm_unbind(struct device *dev, struct device *master,
@@ -474,6 +466,7 @@ static const struct component_ops ipu_crtc_ops = {
 static int ipu_drm_probe(struct platform_device *pdev)
 {
        struct device *dev = &pdev->dev;
+       struct ipu_crtc *ipu_crtc;
        int ret;
 
        if (!dev->platform_data)
@@ -483,6 +476,12 @@ static int ipu_drm_probe(struct platform_device *pdev)
        if (ret)
                return ret;
 
+       ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
+       if (!ipu_crtc)
+               return -ENOMEM;
+
+       dev_set_drvdata(dev, ipu_crtc);
+
        return component_add(dev, &ipu_crtc_ops);
 }
 

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 08fafa4bf8c21302d8d0e464081f6b29281f0291..43e109d67fe3009cea660defce65ada5d3c111ba 100644 (file)
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -330,9 +330,8 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
        u32 bus_format = 0;
        const char *fmt;
 
-       imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
-       if (!imxpd)
-               return -ENOMEM;
+       imxpd = dev_get_drvdata(dev);
+       memset(imxpd, 0, sizeof(*imxpd));
 
        edidp = of_get_property(np, "edid", &imxpd->edid_len);
        if (edidp)
@@ -363,8 +362,6 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
        if (ret)
                return ret;
 
-       dev_set_drvdata(dev, imxpd);
-
        return 0;
 }
 
@@ -386,6 +383,14 @@ static const struct component_ops imx_pd_ops = {
 
 static int imx_pd_probe(struct platform_device *pdev)
 {
+       struct imx_parallel_display *imxpd;
+
+       imxpd = devm_kzalloc(&pdev->dev, sizeof(*imxpd), GFP_KERNEL);
+       if (!imxpd)
+               return -ENOMEM;
+
+       platform_set_drvdata(pdev, imxpd);
+
        return component_add(&pdev->dev, &imx_pd_ops);
 }