Skip to content

Commit

Permalink
drm/imx: fix use after free
Browse files Browse the repository at this point in the history 
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: 17b5001 ("imx-drm: convert to componentised device support")
Signed-off-by: Philipp Zabel <[email protected]>
[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 <[email protected]>
Signed-off-by: Philipp Zabel <[email protected]>
  • Loading branch information
@pH5
pH5 committed Jul 20, 2020
1 parent b3a9e3b commit ba807c9
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 32 deletions.
15 changes: 10 additions & 5 deletions drivers/gpu/drm/imx/dw_hdmi-imx.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,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;
Expand All @@ -235,8 +234,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);

platform_set_drvdata(pdev, hdmi);

hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);

/*
Expand Down Expand Up @@ -266,6 +263,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);
}

Expand Down
3 changes: 2 additions & 1 deletion drivers/gpu/drm/imx/imx-drm-core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,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);
Expand Down
15 changes: 10 additions & 5 deletions drivers/gpu/drm/imx/imx-ldb.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,9 +590,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)) {
Expand Down Expand Up @@ -700,8 +699,6 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
}
}

dev_set_drvdata(dev, imx_ldb);

return 0;

free_child:
Expand Down Expand Up @@ -733,6 +730,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);
}

Expand Down
15 changes: 10 additions & 5 deletions drivers/gpu/drm/imx/imx-tve.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,9 +542,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);
Expand Down Expand Up @@ -655,8 +654,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;
}

Expand All @@ -676,6 +673,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);
}

Expand Down
21 changes: 10 additions & 11 deletions drivers/gpu/drm/imx/ipuv3-crtc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
Expand All @@ -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);
}

Expand Down
15 changes: 10 additions & 5 deletions drivers/gpu/drm/imx/parallel-display.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,9 +326,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)
Expand Down Expand Up @@ -359,8 +358,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;
}

Expand All @@ -382,6 +379,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);
}

Expand Down

0 comments on commit ba807c9

Please sign in to comment.