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

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

New issues:
- [Medium] If debugfs_create_dir() fails, dma_heap_debugfs_dir remains NULL, causing subsequent debugfs files to be erroneously created in the debugfs root directory.
- [Low] The newly added dma_heap_exit function performs partial cleanup by removing debugfs entries but leaks the character device region and class.

Pre-existing issues:
- [Critical] Missing provider module pinning and absence of a heap unregister function causes a Use-After-Free if a dynamically loaded heap provider fails module initialization.
--

--- Patch [6]: [PATCH v4 06/10] dma-buf: heaps: Add debugfs support ---
commit 1c3871b543fe35314ecde7429a59aac59e23b5b3
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..389cddd07c8d3 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c

[ ... ]

> @@ -225,6 +226,46 @@ const char *dma_heap_get_name(struct dma_heap *heap)
>  }
>  EXPORT_SYMBOL_NS_GPL(dma_heap_get_name, "DMA_BUF_HEAP");
>  
> +#ifdef CONFIG_DEBUG_FS
> +static int dma_heap_debug_show(struct seq_file *s, void *unused)
> +{
> +	struct dma_heap *heap = s->private;
> +	int err = 0;
> +
> +	if (heap->ops && heap->ops->show)
> +		err = heap->ops->show(s, heap);
> +
> +	return err;
> +}
> +DEFINE_SHOW_ATTRIBUTE(dma_heap_debug);
> +
> +static struct dentry *dma_heap_debugfs_dir;
> +
> +static void dma_heap_init_debugfs(void)
> +{
> +	struct dentry *dir;
> +
> +	dir = debugfs_create_dir("dma_heap", NULL);
> +	if (IS_ERR(dir))
> +		return;
> +
> +	dma_heap_debugfs_dir = dir;
> +}

[Severity: Medium]
If debugfs_create_dir() fails and returns an error pointer, this check
returns early and leaves dma_heap_debugfs_dir as NULL.

Later, when dma_heap_add() attempts to create files, it will pass NULL
as the parent directory. Does this cause the debugfs files to be erroneously 
created in the debugfs root namespace?

If dma_heap_debugfs_dir was allowed to hold the error pointer instead,
debugfs_create_file() would safely abort child creation.

[ ... ]

> @@ -299,6 +340,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: Critical]
This is a pre-existing issue, but registering debugfs files and character 
devices without a corresponding unregister function seems to expose a 
use-after-free vulnerability. 

If a dynamically loaded heap provider module successfully registers its first 
heap via dma_heap_add() during module_init, but fails to register a second 
heap, the module loader will abort the load and free the module's memory.

Since there is no dma_heap_remove() function to undo the first registration, 
the first heap remains in heap_list and exposed via its debugfs file.

If a user later accesses this debugfs file (or the chardev), will the kernel
dereference the freed heap->ops->show pointer in dma_heap_debug_show()?

[ ... ]

> @@ -335,6 +383,14 @@ 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();
> +}
> +__exitcall(dma_heap_exit);

[Severity: Low]
The new dma_heap_exit() function only cleans up the debugfs entries. Does 
this leak the character device region and the device class allocated earlier 
in dma_heap_init()?

Even though the subsystem is currently built-in only, omitting the cleanup
might become a hazard if it is ever converted to a loadable module.

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