Re: [PATCH v5 06/10] dma-buf: heaps: Add debugfs support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A race condition in `dma_heap_add()` allows userspace to trigger an exploitable use-after-free by opening the character device before the initialization completes.
--

--- Patch [6]: [PATCH v5 06/10] dma-buf: heaps: Add debugfs support ---
commit f236972ccf6e5f7088156d7adcf0aed10121c6c6
Author: Thierry Reding <[email protected]>

dma-buf: heaps: Add debugfs support
    
Add a callback to struct dma_heap_ops that heap providers can implement
to show information about the state of the heap in debugfs. A top-level
directory named "dma_heap" is created in debugfs and individual files
will be named after the heaps.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 3937dd41bb0f6..f279b8fdd3ce0 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
> +

[Severity: High]
This is a pre-existing issue, but is there a race condition in this function
that could lead to a use-after-free?

In dma_heap_add(), cdev_add() is called earlier, making the device accessible.
If the duplicate name check fails, it goes to the error path, destroying the
device and freeing the heap structure:

drivers/dma-buf/dma-heap.c:dma_heap_add() {
    ...
    ret = cdev_add(&heap->heap_cdev, heap->heap_devt, 1);
    ...
    list_for_each_entry(h, &heap_list, list) {
        if (!strcmp(h->name, exp_info->name)) {
            ...
            goto err3;
        }
    }
    ...
err3:
    device_destroy(dma_heap_class, heap->heap_devt);
    ...
err0:
    kfree(heap);
    return err_ret;
}

If a user concurrently opens the device between cdev_add() and the kfree()
on the error path, would the resulting file descriptor retain a dangling
pointer to the freed heap structure in file->private_data?

Could a subsequent ioctl() on this file descriptor then dereference the freed
heap, triggering a use-after-free?

>  	mutex_unlock(&heap_list_lock);
>  
>  	return heap;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6