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 <[email protected]> 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(®ions[i].kobj, &memstat_ktype, > + root, "%s", info->region[i].name); > + if (ret) > + goto err_regions; > + } [ ... ] > +err_regions: > + while (i-- > 0) { > + kobject_del(®ions[i].kobj); > + kobject_put(®ions[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(®ion->kobj); > + kobject_put(®ion->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/[email protected]?part=1
