Re: [PATCH 1/4] rust: debugfs: drop 'static bound from ScopedDir file creation methods

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Unsound transmute in `FileOps::fops` leads to Stack Use-After-Free due to silent constant promotion failure for non-'static types.
- [High] Unsound reliance on RAII (Drop) to manage C API unregistration for non-'static types allows a Use-After-Free via `mem::forget`.
- [Medium] The patch missed dropping the `T: 'static` bound from `ScopedDir`'s callback-based file creation methods.
- [Low] Missing `#[inline]` annotation on the small abstraction function `fops`.
--

commit 7b2fa56e56ab7cdc906ac4f77ac3f200f9ffe65e
Author: Danilo Krummrich <dakr@kernel.org>

rust: debugfs: drop 'static bound from ScopedDir file creation methods

This commit drops the T: 'static bound from various ScopedDir file creation
methods to support registering debugfs files backed by types that contain
non-'static references. It formalizes a type invariant that FileOps instances
reside in static storage and replaces the Deref implementation with an
explicit fops() method.

> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index d7b8014a64746..2beb55d444ca1 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
[ ... ]
> @@ -550,7 +550,7 @@ fn create_file<T: Sync>(&self, name: &CStr, data: &'data T, vtable: &'static Fil
>      /// This function does not produce an owning handle to the file. The created
>      /// file is removed when the [`Scope`] that this directory belongs
>      /// to is dropped.
> -    pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: &CStr, data: &'data T) {
> +    pub fn read_only_file<T: Writer + Send + Sync>(&self, name: &CStr, data: &'data T) {
>          self.create_file(name, data, &T::FILE_OPS)
>      }

[Severity: High]
Does removing the 'static bound introduce a potential use-after-free if the
directory scope is leaked?

If a developer safely leaks a Pin<KBox<Scope<T>>> (e.g., via core::mem::forget)
where T borrows a local stack variable, the Drop-based unregistration will be
skipped. The local variables borrowed by T will still expire, but the C VFS will
retain a pointer to them, leading to a dangling pointer when the debugfs file is
later accessed.

[ ... ]
> @@ -596,11 +592,7 @@ pub fn read_callback_file<T, F>(&self, name: &CStr, data: &'data T, _f: &'static
>      /// This function does not produce an owning handle to the file. The created
>      /// file is removed when the [`Scope`] that this directory belongs
>      /// to is dropped.
> -    pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>(
> -        &self,
> -        name: &CStr,
> -        data: &'data T,
> -    ) {
> +    pub fn read_write_file<T: Writer + Reader + Send + Sync>(&self, name: &CStr, data: &'data T) {
>          let vtable = &<T as ReadWriteFile<_>>::FILE_OPS;
>          self.create_file(name, data, vtable)
>      }

[Severity: Medium]
Was it intended to leave the T: 'static bound in place for the callback-based
methods like read_callback_file() and read_write_callback_file()?

The commit states the goal is to drop the 'static bound from ScopedDir's file
creation methods, but looking at the full file, it appears it was retained on
the callback variants.

[ ... ]
> diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs
> index f15908f71c4a2..7e1dd8c75ad92 100644
> --- a/rust/kernel/debugfs/file_ops.rs
> +++ b/rust/kernel/debugfs/file_ops.rs
[ ... ]
> @@ -65,11 +67,11 @@ pub(super) const fn adapt(&self) -> &FileOps<T::Inner> {
>  }
>  
>  #[cfg(CONFIG_DEBUG_FS)]
> -impl<T> Deref for FileOps<T> {
> -    type Target = bindings::file_operations;
> -
> -    fn deref(&self) -> &Self::Target {
> -        &self.operations
> +impl<T> FileOps<T> {
> +    /// Returns a `'static` reference to the inner `file_operations`.
> +    pub(crate) fn fops(&self) -> &'static bindings::file_operations {

[Severity: Low]
Does this small abstraction function need the #[inline] attribute?

The subsystem coding guidelines specify that small functions in the kernel crate
that forward to a C binding call or wrap basic data access should be annotated
with #[inline].

> +        // SAFETY: By the type invariant, `self` resides in static storage.
> +        unsafe { core::mem::transmute(&self.operations) }
>      }
>  }

[Severity: Critical]
Can this transmute cause a stack use-after-free by extending a short-lived
reference to 'static?

Since the T: 'static bound was dropped from methods like read_only_file(), using
a non-'static T will cause constant promotion to fail when evaluating
&T::FILE_OPS. This creates a temporary FileOps<T> on the local stack instead of
in static storage.

When fops() is called on this stack temporary, the transmute bypasses the actual
lifetime and returns a 'static reference to stack memory. When the stack frame
pops, the C VFS is left holding a dangling pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830193824.471089-1-dakr@kernel.org?part=1