Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] `drm_lease_open` fails to assign `filp->f_mapping`, breaking GEM mmap invalidation and causing a physical page Use-After-Free. - [High] Infinite loop in `drm_lease_open()` due to `get_file_rcu()` usage under a spinlock on a file with zero refcount. - [High] `drm_lease_open()` replaces `filp->f_op` without acquiring a module reference for the new fops, leading to a module refcount underflow. - [High] Incorrect error handling in `drm_expose_lease()` leads to a memory leak and a Use-After-Free race condition. --
commit 256d152bf5555316b6c49d22028de885037d1205 Author: Andrey Erokhin <[email protected]> drm/lease: optionally expose leases as device nodes DRM leases are only returned as anonymous file descriptors. Device managers such as udev and logind therefore cannot discover a lease, assign it to a seat, or hand it to a session through their normal device APIs. > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > index 5d2cf724cbd7b..32a11312d0ce8 100644 > --- a/drivers/gpu/drm/drm_lease.c > +++ b/drivers/gpu/drm/drm_lease.c [ ... ] > +static int drm_expose_lease(struct file *lessee_file, > + struct drm_master *lessee) > +{ [ ... ] > + ret = dev_set_name(kdev, "%s-lessee-%d", dev_name(drm_kdev), > lessee->lessee_id); > + if (ret < 0) > + goto minor_free; > + ret = device_add(kdev); > + if (ret < 0) > + goto device_put; > + > + return 0; > + > +device_put: > + put_device(kdev); > + > +minor_free: > + xa_erase(&drm_exposed_lease_minors_xa, minor); [Severity: High] Does this error path introduce a use-after-free regression and a memory leak? If device_add() fails, the code jumps to device_put and calls put_device(), which frees the exposed structure via drm_exposed_device_release(). Execution then falls through to minor_free and calls xa_erase(). Between the free and the erase, concurrent functions like drm_lease_uevent() can look up the freed exposed pointer in the xarray and dereference it. Additionally, if dev_set_name() fails, the code jumps straight to minor_free, bypassing put_device() and permanently leaking the device structure. > + > + return ret; > +} [ ... ] > +static int drm_lease_open(struct inode *inode, struct file *filp) > +{ > + struct drm_exposed_lease *exposed; > + struct file *lessee_file = NULL; > + > + xa_lock(&drm_exposed_lease_minors_xa); > + exposed = xa_load(&drm_exposed_lease_minors_xa, iminor(inode)); > + if (exposed && exposed->lessee_file && > get_file_rcu(&exposed->lessee_file)) [Severity: High] Can get_file_rcu() cause an infinite loop regression here? If a user opens a lease device concurrently while the anonymous lease file is being closed, the lease file's refcount can drop to 0. The get_file_rcu() function spins indefinitely waiting for the pointer to become NULL if the refcount is 0. Since exposed->lessee_file is never set to NULL, this lockup occurs under the drm_exposed_lease_minors_xa spinlock, preventing the cleanup routine from ever removing it. > + lessee_file = exposed->lessee_file; > + xa_unlock(&drm_exposed_lease_minors_xa); > + > + if (!lessee_file) > + return -ENODEV; > + > + replace_fops(filp, &exposed->fops); [Severity: High] Does replacing the file operations here cause a module reference count underflow regression? The replace_fops() macro correctly drops the reference to the old fops, but this code does not call fops_get(&exposed->fops) to acquire a reference to the new fops owner. When the file is later closed, VFS will call fops_put() and drop a module reference that was never acquired. > + filp->private_data = lessee_file->private_data; [Severity: Critical] Does this initialization miss setting the file mapping, potentially leading to a use-after-free regression of physical memory? The code copies fops and private_data from the lessee_file, but fails to update filp->f_mapping = lessee_file->f_mapping. When userspace mmaps the file, the VMA is attached to the generic character device's address space rather than the DRM device's address space. If the GEM object is later destroyed, the DRM driver's unmap_mapping_range() call targets the DRM address space and fails to find the VMA. This leaves the PTEs intact, allowing userspace to continue accessing freed physical pages. > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
