Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] Incomplete resource cleanup in the newly added `dma_heap_exit()` leaves dynamically allocated heaps, character devices, and sysfs devices active but orphaned. Pre-existing issues: - [High] Race condition in `dma_heap_add` error path leads to a critical Use-After-Free of `struct dma_heap` due to premature device exposure. -- commit e23fa30f3769a236faddd443565a8ccf6a9af125 Author: Thierry Reding <[email protected]> dma-buf: heaps: Add debugfs support This commit adds a callback to struct dma_heap_ops to show heap state in debugfs. It creates a top-level dma_heap directory where individual heap files are registered. A new dma_heap_exit() function is also introduced to clean up the class and debugfs directory on module exit. > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 8a6c2b6e0cc60..65c80dac65009 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c [ ... ] > @@ -299,6 +334,13 @@ struct dma_heap *dma_heap_add(const struct > dma_heap_export_info *exp_info) > > /* Add heap to the list */ > list_add(&heap->list, &heap_list); > + > +#ifdef CONFIG_DEBUG_FS > + if (heap->ops && heap->ops->show) > + debugfs_create_file(heap->name, 0444, dma_heap_debugfs_dir, > + heap, &dma_heap_debug_fops); > +#endif > + > mutex_unlock(&heap_list_lock); > > return heap; [Severity: High] This isn't a bug introduced by this patch, but is it possible for a concurrent userspace process to trigger a use-after-free during the error path of dma_heap_add()? Earlier in dma_heap_add(), the character device is exposed to userspace before the name uniqueness check is performed: ret = cdev_add(&heap->heap_cdev, heap->heap_devt, 1); ... dev_ret = device_create(dma_heap_class, ...); ... /* check the name is unique */ list_for_each_entry(h, &heap_list, list) { if (!strcmp(h->name, exp_info->name)) { ... goto err3; } } If another driver attempts to register a heap with a duplicate name, could a userspace process open the device before the uniqueness check fails? This would store the heap pointer in the open file descriptor. When the uniqueness check then fails, the error path executes kfree(heap), leaving the open file descriptor pointing to freed memory. [ ... ] > @@ -335,6 +377,16 @@ static int dma_heap_init(void) > } > dma_heap_class->devnode = dma_heap_devnode; > > + dma_heap_init_debugfs(); > + > return 0; > } > subsys_initcall(dma_heap_init); > + > +static void __exit dma_heap_exit(void) > +{ > + dma_heap_exit_debugfs(); > + class_destroy(dma_heap_class); [Severity: Medium] Does this successfully tear down the resources if there are dynamically allocated heaps still registered? The dma_heap_exit() function destroys the sysfs class by calling class_destroy(), but it doesn't appear to iterate over heap_list to cleanly tear down the individual registered heaps. Can this leave dangling references and lead to sysfs issues, since device_destroy() and cdev_del() are never called for any active heaps before the class is destroyed? > + unregister_chrdev_region(dma_heap_devt, NUM_HEAP_MINORS); > +} > +__exitcall(dma_heap_exit); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
