[PATCH] drm/exynos: vidi: fix EDID leak on concurrent connection ioctl

[PATCH] drm/exynos: vidi: fix EDID leak on concurrent connection ioctl

[Guangshuo Li]

The change referenced by the Fixes tag added ctx->lock protection around
struct vidi_context members related to EDID allocation and freeing, but
vidi_connection_ioctl() still checks ctx->connected separately from the
state update.

Two authenticated clients can therefore issue concurrent connect
requests. Both can observe ctx->connected as disconnected before either
one updates it, then both allocate a drm_edid and assign ctx->raw_edid.
The second assignment overwrites the first drm_edid pointer, leaking the
first allocation.

Keep the connection-state check and the ctx->raw_edid/ctx->connected
updates in the same critical section. If a concurrent request has already
changed the connection state by the time a newly allocated EDID is ready,
drop the new EDID before returning.

Fixes: 52b330799e2d ("drm/exynos: vidi: use ctx->lock to protect struct vidi_context member variables related to memory alloc/free")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/gpu/drm/exynos/exynos_drm_vidi.c | 33 ++++++++++++++----------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 67bbf9b8bc0e..bc0af9e6bc81 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -252,15 +252,6 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 		return -EINVAL;
 	}
 
-	mutex_lock(&ctx->lock);
-	if (ctx->connected == vidi->connection) {
-		mutex_unlock(&ctx->lock);
-		DRM_DEV_DEBUG_KMS(ctx->dev,
-				  "same connection request.\n");
-		return -EINVAL;
-	}
-	mutex_unlock(&ctx->lock);
-
 	if (vidi->connection) {
 		const struct drm_edid *drm_edid;
 		const void __user *edid_userptr = u64_to_user_ptr(vidi->edid);
@@ -293,21 +284,37 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 					  "edid data is invalid.\n");
 			return -EINVAL;
 		}
+
 		mutex_lock(&ctx->lock);
+		if (ctx->connected == vidi->connection) {
+			mutex_unlock(&ctx->lock);
+			drm_edid_free(drm_edid);
+			DRM_DEV_DEBUG_KMS(ctx->dev,
+					  "same connection request.\n");
+			return -EINVAL;
+		}
+
+		drm_edid_free(ctx->raw_edid);
+
 		ctx->raw_edid = drm_edid;
+		ctx->connected = vidi->connection;
 		mutex_unlock(&ctx->lock);
 	} else {
 		/* with connection = 0, free raw_edid */
 		mutex_lock(&ctx->lock);
+		if (ctx->connected == vidi->connection) {
+			mutex_unlock(&ctx->lock);
+			DRM_DEV_DEBUG_KMS(ctx->dev,
+					  "same connection request.\n");
+			return -EINVAL;
+		}
+
 		drm_edid_free(ctx->raw_edid);
 		ctx->raw_edid = NULL;
+		ctx->connected = vidi->connection;
 		mutex_unlock(&ctx->lock);
 	}
 
-	mutex_lock(&ctx->lock);
-	ctx->connected = vidi->connection;
-	mutex_unlock(&ctx->lock);
-
 	drm_helper_hpd_irq_event(ctx->drm_dev);
 
 	return 0;
-- 
2.43.0

Re: [PATCH] drm/exynos: vidi: fix EDID leak on concurrent connection ioctl

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent execution of `vidi_connection_ioctl` and device removal leads to a Use-After-Free of `ctx`.
--

--- Patch [1]: [PATCH] drm/exynos: vidi: fix EDID leak on concurrent connection ioctl ---
commit a672572794a1e16312bff77e391a1da1f3e7e9fe
Author: Guangshuo Li <lgs201920130244@gmail.com>

drm/exynos: vidi: fix EDID leak on concurrent connection ioctl

This commit fixes a memory leak in vidi_connection_ioctl() caused by
concurrent connection requests. It ensures the connection state check
and the EDID assignment are protected by the same critical section to
prevent a second request from overwriting an already allocated EDID.

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index 67bbf9b8bc0ef..bc0af9e6bc810 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c

[ ... ]

> @@ -293,21 +284,37 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
>  					  "edid data is invalid.\n");
>  			return -EINVAL;
>  		}
> +
>  		mutex_lock(&ctx->lock);

[Severity: High]
This is a pre-existing issue, but can this result in a Use-After-Free if
the device is unbound concurrently?

Looking at the start of vidi_connection_ioctl(), ctx is resolved locklessly:

	struct device *dev = priv ? priv->vidi_dev : NULL;
	struct vidi_context *ctx = dev ? dev_get_drvdata(dev) : NULL;

If an authenticated client calls this ioctl, it can sleep earlier in this
function during copy_from_user() or drm_edid_alloc().

If the exynos-drm-vidi platform device is concurrently unbound via sysfs,
vidi_remove() could complete and devres may free the ctx structure while
the ioctl is sleeping.

Since Exynos DRM uses drm_dev_unregister() instead of drm_dev_unplug()
during teardown, in-flight ioctls are not blocked by the DRM core's
drm_dev_is_unplugged() check.

When the ioctl wakes up and reaches this mutex_lock(&ctx->lock), will it
dereference the freed ctx pointer?

> +		if (ctx->connected == vidi->connection) {
> +			mutex_unlock(&ctx->lock);
> +			drm_edid_free(drm_edid);
> +			DRM_DEV_DEBUG_KMS(ctx->dev,
> +					  "same connection request.\n");
> +			return -EINVAL;
> +		}
> +
> +		drm_edid_free(ctx->raw_edid);
> +
>  		ctx->raw_edid = drm_edid;
> +		ctx->connected = vidi->connection;
>  		mutex_unlock(&ctx->lock);

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