Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Arend van Spriel <arend@broadcom.com>,
        linux-kernel@vger.kernel.org
References: <20210404004504.5547-1-samuel@sholland.org>
 <YGmQAWBN4+uQCIUa@kroah.com>
From:   Samuel Holland <samuel@sholland.org>
Subject: Re: [PATCH] debugfs: Fix use-after-free in
 debugfs_create_devm_seqfile()
Message-ID: <2a36734b-c76c-7554-5f16-913ffebe288f@sholland.org>
Date:   Sun, 4 Apr 2021 12:26:10 -0500
User-Agent: Mozilla/5.0 (X11; Linux ppc64; rv:78.0) Gecko/20100101
 Thunderbird/78.5.1
MIME-Version: 1.0
In-Reply-To: <YGmQAWBN4+uQCIUa@kroah.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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