linux-kernel - Re: [PATCH v5 4/4] rust: samples: Add debugfs sample

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aC7DVewqqWIKetmk@pollux>
Date: Thu, 22 May 2025 08:25:25 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Benno Lossin <lossin@...nel.org>,
	Matthew Maurer <mmaurer@...gle.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Trevor Gross <tmgross@...ch.edu>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	Timur Tabi <ttabi@...dia.com>, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v5 4/4] rust: samples: Add debugfs sample

On Wed, May 21, 2025 at 10:43:26PM +0000, Alice Ryhl wrote:
> On Wed, May 21, 2025 at 09:57:29AM +0200, Danilo Krummrich wrote:
> > On Tue, May 20, 2025 at 09:24:21PM +0000, Alice Ryhl wrote:
> > > * If you remove a directory before the removing objects inside it, then
> > >   the Rust objects become "ghost" objects that are still usable, but not
> > >   visible in the file system anywhere. I.e. calling methods on them
> > >   succeed but are no-ops.
> > 
> > If we would want to keep an entry alive as long as there are more leaves, we'd
> > obviously need to reference count things.
> > 
> > But what do we need reference counting for with this logic? Shouldn't this be
> > also possible without?
> 
> Well, my understanding is that when you remove the parent directory, the
> dentries for child directories and files are freed, so trying to use the
> Rust objects that correspond to those child dentries would be a UAF. I
> want to refcount those child entries so that they at least remain valid
> memory even if they're no longer visible in the file system.

Yes, that makes sense.

Instead of using the dentry pointer as a handle, we could also use the entry's
path and do a lookup whenever the entry is used. Not saying this is better
though.

> > > * Possibly we have a way to drop a Rust object without removing it from
> > >   the file system. In this case, it can never be accessed from Rust
> > >   again, and the only way to remove it is to drop its parent directory.
> > 
> > I'm not sure about this one.
> > 
> > IIUC, this basically brings back the "keep() logic", which I still think is a
> > footgun and should be avoided.
> > 
> > Also, we only needed this, since due to the borrowing design we couldn't store
> > parent and child nodes in the same structure. With reference counting (or the
> > logic above) this goes away.
> > 
> > I wrote the following in a previous conversation [1].
> > 
> > --
> > 
> > What I see more likely to happen is a situation where the "root" directory
> > (almost) lives forever, and hence subsequent calls, such as
> > 
> > 	root.subdir("foo").keep()
> > 
> > effectively are leaks.
> > 
> > One specific example for that would be usb_debug_root, which is created in the
> > module scope of usb-common and is used by USB host / gadget / phy drivers.
> > 
> > The same is true for other cases where the debugfs "root" is created in the
> > module scope, but subsequent entries are created by driver instances. If a
> > driver would use keep() in such a case, we'd effectively leak memory everytime a
> > device is unplugged (or unbound in general).
> 
> Where is the leak? If the file is still visible in the file system, then
> it's not a leak to still have the memory. If the file got removed by
> removing its parent, then my intent is that this should free the memory
> of the child object.

Well, take the case I described above, where the debugfs "root" is created in
the module scope, but subsequent entries are created by driver instances. If a
driver would use keep() in such a case, we'd effectively the file / directory
(and subsequently also the corresponding memory) everytime a device is unplugged
(or unbound in general)."

If the module is built-in the directory from the module scope is *never*
removed, but the entries the driver e.g. creates in probe() for a particular
device with keep() will pile up endlessly, especially for hot-pluggable devices.

(It's getting even worse when there's data bound to such a leaked file, that
might even contain a vtable that is entered from any of the fops of the file.)

That'd be clearly a bug, but for the driver author calling keep() seems like a
valid thing to do -- to me that's clearly a built-in footgun.