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: <2025052205-thing-daylight-1115@gregkh>
Date: Thu, 22 May 2025 10:28:14 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Alice Ryhl <aliceryhl@...gle.com>, 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 Thu, May 22, 2025 at 08:25:25AM +0200, Danilo Krummrich wrote:
> 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.

Either is fine, as long as that "handle" isn't exported out of the
internal binding for anyone to access directly.

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

Yeah, I like the keep() thing less and less and I think it can be done
without it entirely.

thanks,

greg k-h