[PATCH] debugfs: Fix use-after-free in debugfs_create_devm_seqfile()

[PATCH] debugfs: Fix use-after-free in debugfs_create_devm_seqfile()

[Samuel Holland]

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.

Fixes: 98210b7f73f1d ("debugfs: add helper function to create device related seq_file")
Fixes: 0d519cbf38eed ("debugfs: remove return value of debugfs_create_devm_seqfile()")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 fs/debugfs/file.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 686e0ad28788..64f1f918e119 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1100,6 +1100,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_regset32);
 struct debugfs_devm_entry {
 	int (*read)(struct seq_file *seq, void *data);
 	struct device *dev;
+	struct dentry *dentry;
 };
 
 static int debugfs_devm_entry_open(struct inode *inode, struct file *f)
@@ -1117,6 +1118,13 @@ static const struct file_operations debugfs_devm_entry_ops = {
 	.llseek = seq_lseek
 };
 
+static void debugfs_devm_entry_release(struct device *dev, void *res)
+{
+	struct debugfs_devm_entry *entry = res;
+
+	debugfs_remove(entry->dentry);
+}
+
 /**
  * debugfs_create_devm_seqfile - create a debugfs file that is bound to device.
  *
@@ -1136,14 +1144,19 @@ void debugfs_create_devm_seqfile(struct device *dev, const char *name,
 	if (IS_ERR(parent))
 		return;
 
-	entry = devm_kzalloc(dev, sizeof(*entry), GFP_KERNEL);
+	entry = devres_alloc(debugfs_devm_entry_release, sizeof(*entry), GFP_KERNEL);
 	if (!entry)
 		return;
 
 	entry->read = read_fn;
 	entry->dev = dev;
+	entry->dentry = debugfs_create_file(name, S_IRUGO, parent, entry,
+					    &debugfs_devm_entry_ops);
+	if (IS_ERR(entry->dentry)) {
+		devres_free(entry);
+		return;
+	}
 
-	debugfs_create_file(name, S_IRUGO, parent, entry,
-			    &debugfs_devm_entry_ops);
+	devres_add(dev, entry);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_devm_seqfile);
-- 
2.26.2

Re: [PATCH] debugfs: Fix use-after-free in debugfs_create_devm_seqfile()

[Greg Kroah-Hartman]

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 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.

thanks,

greg k-h

Re: [PATCH] debugfs: Fix use-after-free in debugfs_create_devm_seqfile()

[Samuel Holland]

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?

Do you expect the driver to remove the file as a side effect of
recursively removing its parent? 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

> 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.

Samuel

Re: [PATCH] debugfs: Fix use-after-free in debugfs_create_devm_seqfile()

[Greg Kroah-Hartman]

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