[PATCH] drm/mxsfb: lcdif: Fix use-after-free on unbind

[PATCH] drm/mxsfb: lcdif: Fix use-after-free on unbind

[Fabio Estevam]

lcdif allocates its private data, which embeds the CRTC and primary
plane, and its encoders using devres. These DRM objects are linked into
the mode_config lists.

Since mode_config cleanup is DRM-managed, it is deferred until the DRM
device is released. An open DRM file can keep the DRM device alive after
the platform device has been unbound and devres has freed those objects.
The later mode_config cleanup then dereferences freed memory.

Allocate the private data with drmm before initializing mode_config so
that it remains alive until mode_config cleanup has completed. Allocate
encoders with drmm_plain_encoder_alloc(), which cleans up each encoder
and removes it from the mode_config list before freeing its memory.

This can be reproduced on i.MX8MP by keeping the DRM card open across an
LCDIF unbind and closing it afterwards.

Cc: stable@vger.kernel.org
Fixes: 1c71d925c03a ("drm: lcdif: Switch to drmm_mode_config_init")
Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/gpu/drm/mxsfb/lcdif_drv.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
index e2173c4d6fc2..7e9f3b205a45 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
@@ -24,6 +24,7 @@
 #include <drm/drm_fbdev_dma.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_mode_config.h>
 #include <drm/drm_module.h>
 #include <drm/drm_of.h>
@@ -43,10 +44,6 @@ static const struct drm_mode_config_helper_funcs lcdif_mode_config_helpers = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
-static const struct drm_encoder_funcs lcdif_encoder_funcs = {
-	.destroy = drm_encoder_cleanup,
-};
-
 static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
 {
 	struct device *dev = lcdif->drm->dev;
@@ -74,19 +71,16 @@ static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
 					     "Failed to get bridge for endpoint%u\n",
 					     of_ep.id);
 
-		encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
-		if (!encoder)
-			return dev_err_probe(dev, -ENOMEM,
-					     "Failed to allocate encoder for endpoint%u\n",
-					     of_ep.id);
-
-		encoder->possible_crtcs = drm_crtc_mask(&lcdif->crtc);
-		ret = drm_encoder_init(lcdif->drm, encoder, &lcdif_encoder_funcs,
-				       DRM_MODE_ENCODER_NONE, NULL);
-		if (ret)
+		encoder = drmm_plain_encoder_alloc(lcdif->drm, NULL,
+						   DRM_MODE_ENCODER_NONE, NULL);
+		if (IS_ERR(encoder)) {
+			ret = PTR_ERR(encoder);
 			return dev_err_probe(dev, ret,
 					     "Failed to initialize encoder for endpoint%u\n",
 					     of_ep.id);
+		}
+
+		encoder->possible_crtcs = drm_crtc_mask(&lcdif->crtc);
 
 		ret = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 		if (ret)
@@ -131,7 +125,7 @@ static int lcdif_load(struct drm_device *drm)
 	struct lcdif_drm_private *lcdif;
 	int ret;
 
-	lcdif = devm_kzalloc(&pdev->dev, sizeof(*lcdif), GFP_KERNEL);
+	lcdif = drmm_kzalloc(drm, sizeof(*lcdif), GFP_KERNEL);
 	if (!lcdif)
 		return -ENOMEM;
 
-- 
2.43.0

Re: [PATCH] drm/mxsfb: lcdif: Fix use-after-free on unbind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unmapped MMIO Access (Hot-unplug protection)
- [High] Missing Error Handling for pm_runtime_get_sync
--

commit 66de576415540b0c76cf173d95116edc4c840ef4
Author: Fabio Estevam <festevam@gmail.com>

drm/mxsfb: lcdif: Fix use-after-free on unbind
    
This patch fixes a use-after-free issue during LCDIF unbind by allocating
the DRM private data and encoders using drmm APIs instead of devres. This
ensures the DRM objects remain alive until mode config cleanup completes.

> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> index e2173c4d6fc28..7e9f3b205a458 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c

[ ... ]

While reviewing this patch, I noticed a couple of related pre-existing issues
in drivers/gpu/drm/mxsfb/lcdif_kms.c:

> drivers/gpu/drm/mxsfb/lcdif_kms.c:lcdif_crtc_atomic_enable() {
> 	...
> 	clk_set_rate(lcdif->clk, m->clock * 1000);
> 
> 	pm_runtime_get_sync(drm->dev);
> 
> 	lcdif_crtc_mode_set_nofb(new_cstate, new_pstate);
> 	...
> }

[Severity: High]
This is a pre-existing issue, but the return value of pm_runtime_get_sync()
is ignored here.

If this call fails (due to a hardware fault or intermediate PM state), the
driver unconditionally proceeds to execute lcdif_crtc_mode_set_nofb(), which
writes to hardware registers. Can this lead to a kernel panic (Synchronous
External Abort) by accessing unpowered hardware registers?


> drivers/gpu/drm/mxsfb/lcdif_kms.c:lcdif_plane_primary_atomic_update() {
> 	...
> 	paddr = drm_fb_dma_get_gem_addr(new_pstate->fb, new_pstate, 0);
> 	if (paddr) {
> 		writel(lower_32_bits(paddr),
> 		       lcdif->base + LCDC_V8_CTRLDESCL_LOW0_4);
> 		writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> 		       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> 	}
> }

[Severity: High]
This is a pre-existing issue, but could this access the lcdif->base MMIO
region after it has been unmapped by devres?

If a privileged user unbinds the driver via sysfs but a userspace process
keeps the DRM device open, drm_dev_unregister() is called but
drm_dev_unplug() is not. Because drm_dev_is_unplugged() would return false,
an ioctl like DRM_IOCTL_MODE_ATOMIC could still execute and trigger
lcdif_plane_primary_atomic_update(). Would this result in a synchronous
abort/kernel panic when accessing the unmapped memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909210213.946159-1-festevam@gmail.com?part=1