Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Concurrent executions of `vidi_connection_ioctl` can cause a memory leak of the `drm_edid` object due to a time-of-check to time-of-use (TOCTOU) race condition when checking and updating the connection state. -- commit 6f3818ed77cd21c4ace5a636e3cf596ff721c935 Author: Mohammad Shahid <[email protected]> drm/exynos: vidi: use memdup_user() instead of kmalloc() and copy_from_user() This patch simplifies the driver by replacing an open-coded sequence of kmalloc() and copy_from_user() with a single call to memdup_user(). > diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c > b/drivers/gpu/drm/exynos/exynos_drm_vidi.c > index 67bbf9b8bc0ef..183671de711e2 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c > @@ -273,14 +273,9 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, > void *data, > > 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; > - } > + edid_buf = memdup_user(edid_userptr, size); > + if (IS_ERR(edid_buf)) > + return PTR_ERR(edid_buf); > > drm_edid = drm_edid_alloc(edid_buf, size); > kfree(edid_buf); [Severity: High] This isn't a bug introduced by this patch, but is it possible for concurrent ioctl calls to leak the drm_edid object allocated here? Looking at vidi_connection_ioctl() in drivers/gpu/drm/exynos/exynos_drm_vidi.c, the connection state is checked under ctx->lock, but the lock is then released before allocating the EDID: mutex_lock(&ctx->lock); if (ctx->connected == vidi->connection) { mutex_unlock(&ctx->lock); ... } mutex_unlock(&ctx->lock); If multiple threads call vidi_connection_ioctl() concurrently with vidi->connection = 1 while ctx->connected is 0, won't they both pass the initial check, allocate a new drm_edid object from userspace data, and then sequentially re-acquire the lock to assign it? mutex_lock(&ctx->lock); ctx->raw_edid = drm_edid; mutex_unlock(&ctx->lock); Does this unconditionally overwrite ctx->raw_edid, leaking any previously allocated drm_edid from the racing thread? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
