Re: [RFC PATCH v2] drm/lease: optionally expose leases as device nodes

[sashiko-bot@kernel.org]

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 <language.lawyer@gmail.com>

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/20260902180846.2753456-1-language.lawyer@gmail.com?part=1