Re: [RFC 1/2] drm: Allow drivers to report standardized memory stats

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] sysfs: UAF due to empty kobj release function and synchronous kfree
- [High] sysfs: uninitialized memory read in unregister loop leads to out-of-bounds access
- [High] drm: double-free of memstat regions on xa_store failure
- [Medium] sysfs: kobject leak in drm_sysfs_register_memstat on error path
--

commit d035e68d523234171ccd6d024a36f5efdbe545f4
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Subject: drm: Allow drivers to report standardized memory stats

Adds a DRM driver level vfunc to opt-in into reporting standardized memory
stats via sysfs. Each region reports the total size and its current usage in
the total_mb and used_mb files.

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8c0879c336e5f..6302a72631005 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -197,10 +198,15 @@ static int drm_minor_register(struct drm_device *dev, enum drm_minor_type type)
>  	if (ret)
>  		goto err_debugfs;
>  
> +	ret = drm_sysfs_register_memstat(minor);
> +	if (ret)
> +		goto err_kdev;
> +
>  	/* replace NULL with @minor so lookups will succeed from now on */
>  	entry = xa_store(drm_minor_get_xa(type), minor->index, minor, GFP_KERNEL);
>  	if (xa_is_err(entry)) {
>  		ret = xa_err(entry);
> +		drm_sysfs_unregister_memstat(minor);
>  		goto err_debugfs;
>  	}

[Severity: High]
Does this error path introduce a double-free?

If xa_store() fails, this jumps to err_debugfs, which bypasses
device_del(minor->kdev) and leaves the minor device registered.

When the caller eventually cleans up by calling drm_minor_unregister(), it
will call drm_sysfs_unregister_memstat(minor) a second time.

Since the first call doesn't set the sysfs_memstat_regions pointer to NULL,
won't this second call attempt to free the same regions array again?

> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 7b16d32b6ff65..4a8c5b0b458d5 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
[ ... ]
> +static void kobj_memstat_release(struct kobject *kobj)
> +{
> +}

[Severity: Critical]
Is it safe to use an empty release callback here?

An empty release function breaks the kobject lifecycle guarantees. If a
userspace process holds a sysfs file open (like total_mb) when the device is
unregistered, the sysfs core retains an elevated reference to this kobject.

> +
> +static const struct kobj_type memstat_ktype = {
> +	.release = kobj_memstat_release,
> +	.sysfs_ops = &kobj_sysfs_ops,
> +	.default_groups = memstat_groups,
> +};
> +
> +int drm_sysfs_register_memstat(struct drm_minor *minor)
> +{
[ ... ]
> +	regions = kmalloc_objs(*regions, info->num_regions + 1);
> +	if (!regions)
> +		return -ENOMEM;

[Severity: High]
Should this use kzalloc_objs() instead?

This allocation leaves the sentinel element at index info->num_regions
uninitialized.

> +
> +	root = kobject_create_and_add("memstat", &minor->kdev->kobj);
> +	if (!root)
> +		goto err_root;
> +
> +	for (i = 0; i < info->num_regions; i++) {
> +		regions[i].region = i;
> +		regions[i].drm = drm;
> +		ret = kobject_init_and_add(&regions[i].kobj, &memstat_ktype,
> +					   root, "%s", info->region[i].name);
> +		if (ret)
> +			goto err_regions;
> +	}
[ ... ]
> +err_regions:
> +	while (i-- > 0) {
> +		kobject_del(&regions[i].kobj);
> +		kobject_put(&regions[i].kobj);
> +	}

[Severity: Medium]
Does this cleanup loop leak the failing kobject?

If kobject_init_and_add() fails at index i, the code jumps to err_regions.
The while (i-- > 0) loop post-decrements i, skipping the cleanup for the
kobject at index i.

Since the kobject API requires calling kobject_put() even if
kobject_init_and_add() fails, wouldn't this leak the partially initialized
kobject and its allocated name?

[ ... ]
> +void drm_sysfs_unregister_memstat(struct drm_minor *minor)
> +{
> +	struct drm_memstat_attr *region = minor->sysfs_memstat_regions;
> +
> +	while (region && region->kobj.state_initialized) {

[Severity: High]
When iterating here, won't the loop read garbage memory for state_initialized
when it reaches the uninitialized sentinel element?

If the garbage memory happens to be non-zero, this could proceed to call
kobject_del() and kobject_put() on arbitrary memory.

> +		kobject_del(&region->kobj);
> +		kobject_put(&region->kobj);
> +		region++;
> +	}
> +
> +	kfree(minor->sysfs_memstat_regions);

[Severity: Critical]
Since kfree() is called synchronously right after kobject_put(), won't the
entire array be deleted even if the kobjects inside still have active
references?

This seems like it could lead to a use-after-free when userspace later reads
from the open sysfs file, since the empty release function above doesn't
delay the freeing of the memory until all references are dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917120444.86471-1-tvrtko.ursulin@igalia.com?part=1