Message-ID: <2025052344-chariot-giddiness-8f58@gregkh>
Date: Fri, 23 May 2025 11:15:22 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Danilo Krummrich <dakr@...nel.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 Thu, May 22, 2025 at 05:40:43PM +0000, Alice Ryhl wrote:
> On Thu, May 22, 2025 at 04:15:46PM +0200, Greg Kroah-Hartman wrote:
> > > > 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.
> > > 
> > > I mean, for cases such as this, I could imagine that you use `keep()` on
> > > the files stored inside of the driver directory, but don't use it on the
> > > directory. That way, you only have to keep a single reference to an
> > > entire directory around, which may be more convenient.
> > 
> > No, sorry, but debugfs files are "create and forget" type of things.
> > The caller has NO reference back to the file at all in the C version,
> > let's not add that functionality back to the rust side after I spent a
> > long time removing it from the C code :)
> > 
> > If you really want to delete a debugfs file that you have created in the
> > past, then look it up and delete it with the call that is present for
> > that.
> > 
> > The only thing I think that might be worth "keeping" in some form, as an
> > object reference as discussed, is a debugfs directory.
> 
> That could work if we don't have any Rust value for files at all. The
> problem is that if we do have such values, then code like this:
> 
> let my_file = dir.create_file("my_file_name");
> dir.delete_file("my_file_name");
> my_file.do_something();
> 
> would be a UAF on the last line. We have to design the Rust API to avoid
> such UAF, which is why I suggested the ghost objects; the delete_file()
> call leaves my_file in a valid but useless state. And as a ghost object,
> the .do_something() call becomes a no-op since the file is now missing
> from the filesystem.

See my other response now, but I don't want there to be any
.do_something() calls that are possible here at all.  All a debugfs file
object can do is be created, or be destroyed.

thanks,

greg k-h

Powered by blists - more mailing lists