linux-kernel - Re: [PATCH v11 2/7] rust: debugfs: Add support for read-only files

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: <DCNGJMN80Z34.1O45B1LM9PB2S@kernel.org>
Date: Mon, 08 Sep 2025 15:36:46 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Cc: "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>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Rafael J. Wysocki"
 <rafael@...nel.org>, "Sami Tolvanen" <samitolvanen@...gle.com>, "Timur
 Tabi" <ttabi@...dia.com>, "Benno Lossin" <lossin@...nel.org>, "Dirk Beheme"
 <dirk.behme@...bosch.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v11 2/7] rust: debugfs: Add support for read-only files

On Mon Sep 8, 2025 at 3:30 PM CEST, Greg Kroah-Hartman wrote:
> On Mon, Sep 08, 2025 at 03:22:41PM +0200, Danilo Krummrich wrote:
>> On Mon Sep 8, 2025 at 2:48 PM CEST, Greg Kroah-Hartman wrote:
>> > On Mon, Sep 08, 2025 at 12:54:46PM +0200, Danilo Krummrich wrote:
>> >> diff --git a/samples/rust/rust_debugfs.rs b/samples/rust/rust_debugfs.rs
>> >> index b26eea3ee723..475502f30b1a 100644
>> >> --- a/samples/rust/rust_debugfs.rs
>> >> +++ b/samples/rust/rust_debugfs.rs
>> >> @@ -59,6 +59,8 @@ struct RustDebugFs {
>> >>      #[pin]
>> >>      _compatible: File<CString>,
>> >>      #[pin]
>> >> +    _test: File<&'static CStr>,
>> >> +    #[pin]
>> >>      counter: File<AtomicUsize>,
>> >>      #[pin]
>> >>      inner: File<Mutex<Inner>>,
>> >> @@ -140,6 +142,7 @@ fn new(pdev: &platform::Device<Core>) -> impl PinInit<Self, Error> + '_ {
>> >>                          .property_read::<CString>(c_str!("compatible"))
>> >>                          .required_by(dev)?,
>> >>                  ),
>> >> +                _test <- debugfs.read_only_file(c_str!("test"), c_str!("some_value")),
>> >
>> > Cool, but again, we do not want to ever be storing individual debugfs
>> > files.  Well, we can, but for 90% of the cases, we do not, we only want
>> > to remove the whole directory when that goes out of scope, which will
>> > clean up the files then.
>> 
>> This API does not work in the way that you have a struct storing the data you
>> want to expose *and* another one for the files with the data attached.
>> 
>> The File type contains the actual data. For instance, if you have a struct Foo,
>> where you want to expose the members through debugfs you would *not* do:
>> 
>> 	struct Foo {
>> 	   a: u32,
>> 	   b: u32,
>> 	}
>> 
>> 	struct FooFiles {
>> 	   a: File<&u32>,
>> 	   b: File<&u32>
>> 	}
>> 
>> and then create an instance of Foo *and* another instance of FooFiles to export
>> them via debugfs.
>
> Ah, that's exactly what I was trying to do.

But that's bad, then we're back at the lifetime problem from the beginning,
because the File<&Foo> then somehow needs to ensure that the instance Foo
remains alive as long as File<&Foo> or the backing directory exists.

So, you eventually end of with Foo needing to be reference counted with its own
memory allocation, which horribly messes with your lifetimes in the driver.

You don't want a a field to be reference counted just because it's exposed via
debugfs.

>> Instead you would change your struct Foo to just be:
>> 
>> 	struct Foo {
>> 	   a: File<u32>,
>> 	   b: File<u32>,
>> 	}
>> 
>> If you now create an instance of Foo (let's call it `foo`), then foo.a or foo.b
>> dereferences to the inner type, i.e. the u32. Or in other words `foo` still
>> behaves as if `a` and `b` would be u32 values. For instance:
>> 
>>    if foo.a == 42 {
>>       pr_info!("Foo::b = {}\n", foo.b);
>>    }
>
> Oh that's not going to work well at all :(
>
> Think about something "simple" like a pci config descriptor.  You have a
> structure, with fields, already sitting there.  You want to expose those
> fields in debugfs.

This is more of a special case that is addressed by the Scope API in patch 6 and
patch 7, so we should be good.

>> The fact that the backing files of `a` and `b` are removed from debugfs when Foo
>> is dropped is necessary since otherwise we create a UAF.
>
> That's fine, but:
>
>> Think of File<T> as a containers like you think of KBox<T>.
>
> Ok, but again, you are now forcing all users to think of debugfs as the
> main "interface" to those variables, which is not true (nor should it
> be.)
>
>> KBox<T> behaves exactly like T, but silently manages the backing kmalloc()
>> allocation that T lives in.
>> 
>> With File<T> it's exactly the same, it behaves exactly like the T that lives
>> within File<T>, but silently manages the debugfs file the T is exposed by.
>
> And what happens if debugfs is not enabled?  What about if creating the
> file fails?  The variable still needs to be present and active and
> working.

This is the case, the variable will still be present and active in any case.