linux-kernel - Re: [PATCH] debugfs: Fix use-after-free in debugfs_create_devm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YGqtHb/CMbiE6w0c@kroah.com>
Date:   Mon, 5 Apr 2021 08:24:29 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Samuel Holland <samuel@...lland.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Arend van Spriel <arend@...adcom.com>,
        linux-kernel@...r.kernel.org
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