Messages in this thread | | | Date | Mon, 5 Apr 2021 08:24:29 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH] debugfs: Fix use-after-free in debugfs_create_devm_seqfile() |
| |
On Sun, Apr 04, 2021 at 12:26:10PM -0500, Samuel Holland wrote: > On 4/4/21 5:08 AM, Greg Kroah-Hartman wrote: > > On Sat, Apr 03, 2021 at 07:45:04PM -0500, Samuel Holland wrote: > >> This function uses devres to clean up its allocation, but it never removes the > >> file referencing that allocation. This causes a use-after-free and an oops if > >> the file is accessed after the owning device is removed. > > > > What in-kernel user of this is having this problem? > > > > The driver should clean up the debugfs file, it is not the debugfs > > core's job to auto-remove the file. > > The function returns void. debugfs_remove() requires the dentry pointer, > which the caller does not have. How is the driver expected to clean up > the file?
Like it does for any other debugfs function (you will not that almost all of them return void now.)
You remove the parent directory.
> Do you expect the driver to remove the file as a side effect of > recursively removing its parent?
Yes.
> If so, that conflicts with the documentation for > debugfs_create_devm_seqfile(), which describes NULL as > a valid parent: > > @parent: a pointer to the parent dentry for this file. This should be a > directory dentry if set. If this parameter is %NULL, then the > file will be created in the root of the debugfs filesystem. > > There is one in-kernel caller that uses a NULL parent, in > drivers/gpio/gpio-tegra.c
That code should be fixed up, putting a driver-specific file in the root of debugfs is a bit rude.
> > The resource is what is being cleaned up by the devm usage in debugfs, > > that's all, not the file. > > > > Please fix up the driver that is creating the file but then not removing > > it. > > In that case, the function documentation should be modified to state > that the driver is responsible for removing the parent directory, and > that NULL is not a valid parent here. I can send a patch doing that instead.
Please feel free to fix up allmost of the debugfs functions as I think they all say the same thing :)
I've been slowly cleaning up the debugfs api, but have not been paying that much attention to the documentation yet, sorry.
thanks,
greg k-h
| |