This is a series of patches that address several memory bugs that occur in the Exynos Virtual Display driver.
Jeongjun Park (3): drm/exynos: vidi: use priv->vidi_dev for ctx lookup in vidi_connection_ioctl() drm/exynos: vidi: fix to avoid directly dereferencing user pointer drm/exynos: vidi: use ctx->lock to protect struct vidi_context member variables related to memory alloc/free
drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + drivers/gpu/drm/exynos/exynos_drm_vidi.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 64 insertions(+), 11 deletions(-)
vidi_connection_ioctl() retrieves the driver_data from drm_dev->dev to obtain a struct vidi_context pointer. However, drm_dev->dev is the exynos-drm master device, and the driver_data contained therein is not the vidi component device, but a completely different device.
This can lead to various bugs, ranging from null pointer dereferences and garbage value accesses to, in unlucky cases, out-of-bounds errors, use-after-free errors, and more.
To resolve this issue, we need to store/delete the vidi device pointer in exynos_drm_private->vidi_dev during bind/unbind, and then read this exynos_drm_private->vidi_dev within ioctl() to obtain the correct struct vidi_context pointer.
Cc: stable@vger.kernel.org Signed-off-by: Jeongjun Park aha310510@gmail.com --- drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + drivers/gpu/drm/exynos/exynos_drm_vidi.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 23646e55f142..06c29ff2aac0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -199,6 +199,7 @@ struct drm_exynos_file_private { struct exynos_drm_private { struct device *g2d_dev; struct device *dma_dev; + struct device *vidi_dev; void *mapping;
/* for atomic commit */ diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c index e094b8bbc0f1..1fe297d512e7 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -223,9 +223,14 @@ ATTRIBUTE_GROUPS(vidi); int vidi_connection_ioctl(struct drm_device *drm_dev, void *data, struct drm_file *file_priv) { - struct vidi_context *ctx = dev_get_drvdata(drm_dev->dev); + struct exynos_drm_private *priv = drm_dev->dev_private; + struct device *dev = priv ? priv->vidi_dev : NULL; + struct vidi_context *ctx = dev ? dev_get_drvdata(dev) : NULL; struct drm_exynos_vidi_connection *vidi = data;
+ if (!ctx) + return -ENODEV; + if (!vidi) { DRM_DEV_DEBUG_KMS(ctx->dev, "user data for vidi is null.\n"); @@ -371,6 +376,7 @@ static int vidi_bind(struct device *dev, struct device *master, void *data) { struct vidi_context *ctx = dev_get_drvdata(dev); struct drm_device *drm_dev = data; + struct exynos_drm_private *priv = drm_dev->dev_private; struct drm_encoder *encoder = &ctx->encoder; struct exynos_drm_plane *exynos_plane; struct exynos_drm_plane_config plane_config = { 0 }; @@ -378,6 +384,8 @@ static int vidi_bind(struct device *dev, struct device *master, void *data) int ret;
ctx->drm_dev = drm_dev; + if (priv) + priv->vidi_dev = dev;
plane_config.pixel_formats = formats; plane_config.num_pixel_formats = ARRAY_SIZE(formats); @@ -423,8 +431,12 @@ static int vidi_bind(struct device *dev, struct device *master, void *data) static void vidi_unbind(struct device *dev, struct device *master, void *data) { struct vidi_context *ctx = dev_get_drvdata(dev); + struct drm_device *drm_dev = data; + struct exynos_drm_private *priv = drm_dev->dev_private;
timer_delete_sync(&ctx->timer); + if (priv) + priv->vidi_dev = NULL; }
static const struct component_ops vidi_component_ops = { --
In vidi_connection_ioctl(), vidi->edid(user pointer) is directly dereferenced in the kernel.
This allows arbitrary kernel memory access from the user space, so instead of directly accessing the user pointer in the kernel, we should modify it to copy edid to kernel memory using copy_from_user() and use it.
Cc: stable@vger.kernel.org Signed-off-by: Jeongjun Park aha310510@gmail.com --- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c index 1fe297d512e7..601406b640c7 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -251,13 +251,27 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
if (vidi->connection) { const struct drm_edid *drm_edid; - const struct edid *raw_edid; + const void __user *edid_userptr = u64_to_user_ptr(vidi->edid); + void *edid_buf; + struct edid hdr; size_t size;
- raw_edid = (const struct edid *)(unsigned long)vidi->edid; - size = (raw_edid->extensions + 1) * EDID_LENGTH; + if (copy_from_user(&hdr, edid_userptr, sizeof(hdr))) + return -EFAULT;
- drm_edid = drm_edid_alloc(raw_edid, size); + size = (hdr.extensions + 1) * EDID_LENGTH; + + edid_buf = kmalloc(size, GFP_KERNEL); + if (!edid_buf) + return -ENOMEM; + + if (copy_from_user(edid_buf, edid_userptr, size)) { + kfree(edid_buf); + return -EFAULT; + } + + drm_edid = drm_edid_alloc(edid_buf, size); + kfree(edid_buf); if (!drm_edid) return -ENOMEM;
--
Exynos Virtual Display driver performs memory allocation/free operations without lock protection, which easily causes concurrency problem.
For example, use-after-free can occur in race scenario like this: ``` CPU0 CPU1 CPU2 ---- ---- ---- vidi_connection_ioctl() if (vidi->connection) // true drm_edid = drm_edid_alloc(); // alloc drm_edid ... ctx->raw_edid = drm_edid; ... drm_mode_getconnector() drm_helper_probe_single_connector_modes() vidi_get_modes() if (ctx->raw_edid) // true drm_edid_dup(ctx->raw_edid); if (!drm_edid) // false ... vidi_connection_ioctl() if (vidi->connection) // false drm_edid_free(ctx->raw_edid); // free drm_edid ... drm_edid_alloc(drm_edid->edid) kmemdup(edid); // UAF!! ... ```
To prevent these vulns, at least in vidi_context, member variables related to memory alloc/free should be protected with ctx->lock.
Cc: stable@vger.kernel.org Signed-off-by: Jeongjun Park aha310510@gmail.com --- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 38 ++++++++++++++++++++---- 1 file changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c index 601406b640c7..37733f2ac0e7 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -186,29 +186,37 @@ static ssize_t vidi_store_connection(struct device *dev, const char *buf, size_t len) { struct vidi_context *ctx = dev_get_drvdata(dev); - int ret; + int ret, new_connected;
- ret = kstrtoint(buf, 0, &ctx->connected); + ret = kstrtoint(buf, 0, &new_connected); if (ret) return ret; - - if (ctx->connected > 1) + if (new_connected > 1) return -EINVAL;
+ mutex_lock(&ctx->lock); + /* * Use fake edid data for test. If raw_edid is set then it can't be * tested. */ if (ctx->raw_edid) { DRM_DEV_DEBUG_KMS(dev, "edid data is not fake data.\n"); - return -EINVAL; + ret = -EINVAL; + goto fail; }
+ ctx->connected = new_connected; + mutex_unlock(&ctx->lock); + DRM_DEV_DEBUG_KMS(dev, "requested connection.\n");
drm_helper_hpd_irq_event(ctx->drm_dev);
return len; +fail: + mutex_unlock(&ctx->lock); + return ret; }
static DEVICE_ATTR(connection, 0644, vidi_show_connection, @@ -243,11 +251,14 @@ 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; @@ -281,14 +292,21 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data, "edid data is invalid.\n"); return -EINVAL; } + mutex_lock(&ctx->lock); ctx->raw_edid = drm_edid; + mutex_unlock(&ctx->lock); } else { /* with connection = 0, free raw_edid */ + mutex_lock(&ctx->lock); drm_edid_free(ctx->raw_edid); ctx->raw_edid = NULL; + 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; @@ -303,7 +321,7 @@ static enum drm_connector_status vidi_detect(struct drm_connector *connector, * connection request would come from user side * to do hotplug through specific ioctl. */ - return ctx->connected ? connector_status_connected : + return READ_ONCE(ctx->connected) ? connector_status_connected : connector_status_disconnected; }
@@ -326,11 +344,15 @@ static int vidi_get_modes(struct drm_connector *connector) const struct drm_edid *drm_edid; int count;
+ mutex_lock(&ctx->lock); + if (ctx->raw_edid) drm_edid = drm_edid_dup(ctx->raw_edid); else drm_edid = drm_edid_alloc(fake_edid_info, sizeof(fake_edid_info));
+ mutex_unlock(&ctx->lock); + drm_edid_connector_update(connector, drm_edid);
count = drm_edid_connector_add_modes(connector); @@ -482,9 +504,13 @@ static void vidi_remove(struct platform_device *pdev) { struct vidi_context *ctx = platform_get_drvdata(pdev);
+ mutex_lock(&ctx->lock); + drm_edid_free(ctx->raw_edid); ctx->raw_edid = NULL;
+ mutex_unlock(&ctx->lock); + component_del(&pdev->dev, &vidi_component_ops); }
--
linux-stable-mirror@lists.linaro.org
-
Jeongjun Park