Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Exposed lease devices are not unregistered when the primary DRM device is unregistered, causing a resource leak of the hardware device. - [High] A race condition in `drm_expose_lease` exposes the lease device to concurrent lookups before initialization completes, leading to a Use-After-Free on error paths. --
commit 384e9d119c9161556cff8b6b1ef37502f82fc82f 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 cannot discover these leases, assign them to seats, or hand them to sessions via their device APIs. Accept O_CREAT in DRM_IOCTL_MODE_CREATE_LEASE to expose the lease as a DRM class device named after its primary node and lessee ID. Make the device a sibling of the primary node with DEVTYPE=drm_lease. > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > index 5d2cf724cbd7b..fdd706530d7c8 100644 > --- a/drivers/gpu/drm/drm_lease.c > +++ b/drivers/gpu/drm/drm_lease.c [ ... ] > @@ -263,10 +271,144 @@ static struct drm_master *drm_lease_create(struct > drm_master *lessor, struct idr > return ERR_PTR(error); > } > > +struct drm_exposed_lease { > + struct file *lessee_file; > + struct drm_master *lessee; > + struct file_operations fops; > + struct device kdev; > +}; [ ... ] > +static int drm_expose_lease(struct file *lessee_file, > + struct drm_master *lessee) > +{ > + int ret = 0; > + u32 minor; > + struct drm_exposed_lease *exposed; > + struct device *drm_kdev; > + struct device *kdev; > + > + exposed = kzalloc_obj(*exposed); > + if (!exposed) > + return -ENOMEM; > + > + exposed->lessee_file = lessee_file; > + exposed->lessee = lessee; > + exposed->fops = *lessee_file->f_op; > + exposed->fops.release = drm_exposed_lease_release; > + > + drm_kdev = lessee->dev->primary->kdev; > + kdev = &exposed->kdev; > + > + device_initialize(kdev); > + kdev->class = drm_kdev->class; > + kdev->type = &drm_exposed_lease_device_type; > + kdev->parent = drm_kdev->parent; > + kdev->release = drm_exposed_device_release; > + ret = dev_set_name(kdev, "%s-lessee-%d", dev_name(drm_kdev), > lessee->lessee_id); > + if (ret < 0) > + goto device_put; > + > + ret = xa_alloc(&drm_exposed_lease_minors_xa, &minor, exposed, > + XA_LIMIT(0, DRM_EXPOSED_LEASE_MINORS - 1), GFP_KERNEL); [Severity: High] This makes the exposed struct globally visible in the xarray before device_add() has completed. Could a concurrent thread guess the minor number and call open() on the newly created lease device node while it is still being initialized? > + if (ret < 0) > + goto device_put; > + > + kdev->devt = MKDEV(drm_exposed_lease_major, minor); > + ret = device_add(kdev); [Severity: High] When the primary DRM device is unregistered (e.g., via drm_dev_unregister() on a hot-unplug), how are these exposed lease devices cleaned up? Since this lease device takes a kobject reference to its parent (kdev->parent = drm_kdev->parent), if the lease devices are not explicitly unregistered during primary device teardown, could this pin the parent kobject indefinitely and leak the physical hardware device structures? > + if (ret < 0) > + goto minor_free; > + > + return 0; > + > +minor_free: > + xa_erase(&drm_exposed_lease_minors_xa, minor); > + > +device_put: > + put_device(kdev); [Severity: High] If device_add() fails and we enter this error path, put_device(kdev) will free the exposed struct. If a concurrent thread had already looked up the minor and passed it to drm_lease_open(), wouldn't this free cause a dangling pointer in the concurrent open()? > + > + return ret; > +} [ ... ] > +static int drm_lease_open(struct inode *inode, struct file *filp) > +{ > + struct drm_exposed_lease *exposed; > + struct file *lessee_file = NULL; > + const struct file_operations *fops = NULL; > + > + xa_lock(&drm_exposed_lease_minors_xa); > + exposed = xa_load(&drm_exposed_lease_minors_xa, iminor(inode)); > + if (exposed && exposed->lessee_file) { > + lessee_file = get_file_active(&exposed->lessee_file); > + if (lessee_file) > + fops = fops_get(&exposed->fops); > + } > + xa_unlock(&drm_exposed_lease_minors_xa); > + > + if (!lessee_file) > + return -ENODEV; > + if (!fops) { > + fput(lessee_file); > + return -ENODEV; > + } > + > + replace_fops(filp, fops); [Severity: High] If drm_expose_lease() fails at device_add() and frees the exposed struct after this concurrent drm_lease_open() has called replace_fops(filp, fops), won't filp->f_op point to freed memory? Could this lead to a Use-After-Free on subsequent syscalls (like ioctl or close)? > + filp->f_mapping = lessee_file->f_mapping; > + filp->private_data = lessee_file->private_data; > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
