[PATCH 0/1] rust: io: Fix `Region::drop` trying to release nested resources from the wrong parent

[PATCH 0/1] rust: io: Fix `Region::drop` trying to release nested resources from the wrong parent

[Priya Bala Govindasamy]

Dear Linux kernel maintainers,

We are developing a tool called FerroLens to detect potential
unsound behavior in Rust code in the Linux kernel. The tool
internally uses an LLM to detect bugs.
We then perform manual analysis to verify these reports.
FerroLens reported the following bug in rust/kernel/io/resource.rs:

Io resource regions can be requested beneath a specified parent 
resource. `Resource::request_region()` passes self as the parent to 
`__request_region()`. But `Region` does not retain that parent.

`Region::drop()` calls `release_mem_region()` or `release_region()`
to release the `Resource`.
Both of these functions start searching for the region to release 
from the global resource root. They also do not search inside busy 
regions. So they may not find a child region if it is nested 
under a busy parent. This leaves the child still allocated. But the 
remaining fields of the child `Region` are freed, including the 
`name` Cstring that it owned. 

Therefore, for nested regions, `Region::drop()` may fail to find 
and release the child region. 
This can lead to resource leaks and leaving the name pointer dangling.

Here is a PoC to demonstrate this issue:

// SPDX-License-Identifier: GPL-2.0

//! Demonstrate that `Region::drop` releases relative to the global resource
//! root instead of the parent passed to `Resource::request_region`.

use core::ptr;

use kernel::{
    bindings,
    io::{
        resource::Flags,
        PhysAddr, Resource, ResourceSize,
    },
    str::CStr,
    prelude::*,
};

module! {
    type: ResourceParentPoc,
    name: "resource_parent_poc",
    authors: ["Priya Govindasamy"],
    description: "PoC for releasing a nested Region with the wrong parent",
    license: "GPL",
    params: {
        start: u64 {
            default: 0x1000_0000,
            description: "Start of the temporary 4 KiB iomem reservation",
        },
    },
}

const PARENT_SIZE: ResourceSize = 0x1000;
const CHILD_OFFSET: PhysAddr = 0x100;
const CHILD_SIZE: ResourceSize = 0x100;

/// Obtain the underlying C resource pointer from its transparent Rust wrapper.
fn resource_to_raw(resource: &Resource) -> *mut bindings::resource {
    ptr::from_ref(resource).cast_mut().cast()
}

struct ResourceParentPoc;

impl kernel::Module for ResourceParentPoc {
    fn init(_module: &'static ThisModule) -> Result<Self> {
        let start: PhysAddr = module_parameters::start
            .value()
            .try_into()
            .map_err(|_| EINVAL)?;
        let child_start = start.checked_add(CHILD_OFFSET).ok_or(EINVAL)?;

        // Check the complete parent interval too, since __request_region uses
        // `start + size - 1` internally.
        start.checked_add(PARENT_SIZE - 1).ok_or(EINVAL)?;

        // `Resource::from_raw` is crate-private. This is the equivalent cast
        // for this out-of-tree PoC.
        let iomem_root_ptr = ptr::addr_of_mut!(bindings::iomem_resource);
        // SAFETY:
        // - `iomem_resource` is a permanent, valid C `struct resource`.
        // - `Resource` is a transparent wrapper around that type.
        // - `Resource` uses interior mutability for access to the C object.
        let iomem_root = unsafe { &*iomem_root_ptr.cast::<Resource>() };

        // This parent is a normal busy region reachable from iomem_resource.
        // If the selected address conflicts with another busy resource, load
        // the module with a different `start=` value.
        let parent = iomem_root
            .request_region(
                start,
                PARENT_SIZE,
                c"resource-parent-poc-parent".to_cstring()?,
                Flags::IORESOURCE_MEM,
            )
            .ok_or(EBUSY)?;

        let parent_raw = resource_to_raw(&parent);

        // This call is safe Rust. Because Region dereferences to Resource, it
        // requests a busy child directly underneath the busy parent.
        let child = parent
            .request_region(
                child_start,
                CHILD_SIZE,
                c"resource-parent-poc-child".to_cstring()?,
                Flags::IORESOURCE_MEM,
            )
            .ok_or(EBUSY)?;

        let child_raw = resource_to_raw(&child);

        pr_info!(
            "resource_parent_poc: parent={:#x}-{:#x}, child={:#x}-{:#x}\n",
            start,
            start + PARENT_SIZE - 1,
            child_start,
            child_start + CHILD_SIZE - 1
        );
        pr_info!(
            "resource_parent_poc: dropping child; Region::drop will search from iomem_resource\n"
        );

        // __release_region starts at iomem_resource, encounters the enclosing
        // busy parent, refuses to descend into it, and emits:
        //
        //   Trying to free nonexistent resource <...>
        //
        // Consequently the child remains linked under `parent`.
        drop(child);

        // SAFETY: `parent_raw` is still owned by `parent`, which is alive, and
        // no other code modifies this private child list during this PoC.
        let child_was_not_released = unsafe { (*parent_raw).child == child_raw };
        if !child_was_not_released {
            pr_err!("resource_parent_poc: child unexpectedly disappeared from its parent\n");
            return Err(EIO);
        }

        pr_info!(
            "resource_parent_poc: confirmed: child is still linked under the original parent\n"
        );

        //check if child's name was freed
        let name_ptr = unsafe { (*child_raw).name };

        if name_ptr.is_null() {
            pr_info!("resource_parent_poc: accessing the child's name now is: (null)\n");
        } else {
            let name = unsafe { CStr::from_char_ptr(name_ptr) };
            pr_info!("resource_parent_poc: accessing the child's name now is: {}\n", name);
        }

        // Clean up the allocation that Region::drop failed to release. Unlike
        // Region::drop, this supplies the parent used for the child request.
        //
        // SAFETY:
        // - `parent_raw` points to the live parent region.
        // - The exact child interval is currently a busy region immediately
        //   underneath that parent, as checked above.
        unsafe { bindings::__release_region(parent_raw, child_start, CHILD_SIZE) };

        // SAFETY: `parent_raw` remains valid until `parent` is dropped below.
        if !unsafe { (*parent_raw).child.is_null() } {
            pr_err!("resource_parent_poc: explicit child cleanup failed\n");
            return Err(EIO);
        }

        pr_info!("resource_parent_poc: explicit cleanup with the correct parent succeeded\n");

        // This parent was requested relative to iomem_resource, so its normal
        // Region::drop path is correct now that it has no child.
        drop(parent);

        Ok(Self)
    }
}

impl Drop for ResourceParentPoc {
    fn drop(&mut self) {
        pr_info!("resource_parent_poc: unloaded\n");
    }
}



Output:
[423973.510861] resource_parent_poc: resource_parent_poc: parent=0xff000000-0xff000fff, child=0xff000100-0xff0001ff
[423973.510929] resource_parent_poc: resource_parent_poc: dropping child; Region::drop will search from iomem_resource
[423973.510985] resource: Trying to free nonexistent resource <0x00000000ff000100-0x00000000ff0001ff>
[423973.511029] resource_parent_poc: resource_parent_poc: confirmed: child is still linked under the original parent
[423973.511032] resource_parent_poc: resource_parent_poc: accessing the child's name now is: 
[423973.511055] resource_parent_poc: resource_parent_poc: explicit cleanup with the correct parent succeeded


Priya Bala Govindasamy (1):
  rust: io: Fix `Region::drop` releasing nested resources from the wrong
    parent

 rust/kernel/io/mem.rs      |  4 ++--
 rust/kernel/io/resource.rs | 38 ++++++++++++++++++++------------------
 2 files changed, 22 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/1] rust: io: Fix `Region::drop` trying to release nested resources from the wrong parent

[Priya Bala Govindasamy]

Io resource regions can be requested beneath a specified parent 
resource.
But `Region::drop` does not consider the parent resource.
It releases regions relative to the global resource root.
For nested regions, it may fail to find and release the child region.
This can lead to resource leaks and leaving the name pointer dangling.

Fix this by storing a reference to the parent resource in `Region`.
This ensures that the parent remains valid for the lifetime of the
child region.
The `Region::drop` implementation is updated to use the parent
resource when releasing the region.

Fixes: 493fc33ec252 ("rust: io: add resource abstraction")
Reported-by: Dylan Zueck<dzueck@uci.edu>
Assisted-by: ChatGPT:gpt-5.6-sol
Signed-off-by: Priya Bala Govindasamy<pgovind2@uci.edu>
---
 rust/kernel/io/mem.rs      |  4 ++--
 rust/kernel/io/resource.rs | 38 ++++++++++++++++++++------------------
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
index 32a919099dcd..245d8e1e5e6c 100644
--- a/rust/kernel/io/mem.rs
+++ b/rust/kernel/io/mem.rs
@@ -173,7 +173,7 @@ pub struct ExclusiveIoMem<'a, const SIZE: usize> {
     /// range represented by the underlying `iomem`.
     ///
     /// This field is needed for ownership of the region.
-    _region: Region,
+    _region: Region<'a>,
 }
 
 impl<const SIZE: usize> ForLt for ExclusiveIoMem<'static, SIZE> {
@@ -191,7 +191,7 @@ unsafe impl<const SIZE: usize> CovariantForLt for ExclusiveIoMem<'static, SIZE>
 
 impl<'a, const SIZE: usize> ExclusiveIoMem<'a, SIZE> {
     /// Creates a new `ExclusiveIoMem` instance.
-    fn ioremap(dev: &'a Device<Bound>, resource: &Resource) -> Result<Self> {
+    fn ioremap(dev: &'a Device<Bound>, resource: &'a Resource) -> Result<Self> {
         let start = resource.start();
         let size = resource.size();
         let name = resource.name().unwrap_or_default();
diff --git a/rust/kernel/io/resource.rs b/rust/kernel/io/resource.rs
index 17b0c174cfc5..0a94a4eb3e7c 100644
--- a/rust/kernel/io/resource.rs
+++ b/rust/kernel/io/resource.rs
@@ -27,15 +27,19 @@
 ///
 /// - `self.0` points to a valid `bindings::resource` that was obtained through
 ///   `bindings::__request_region`.
-pub struct Region {
+pub struct Region<'a> {
     /// The resource returned when the region was requested.
     resource: NonNull<bindings::resource>,
+    /// Parent supplied to `__request_region`.
+    /// The reference also prevents a parent `Region` from being dropped
+    /// before this child.
+    parent: &'a Resource,
     /// The name that was passed in when the region was requested. We need to
     /// store it for ownership reasons.
     _name: CString,
 }
 
-impl Deref for Region {
+impl Deref for Region<'_> {
     type Target = Resource;
 
     fn deref(&self) -> &Self::Target {
@@ -44,31 +48,28 @@ fn deref(&self) -> &Self::Target {
     }
 }
 
-impl Drop for Region {
+impl Drop for Region<'_> {
     fn drop(&mut self) {
-        let (flags, start, size) = {
+        let (start, size) = {
             let res = &**self;
-            (res.flags(), res.start(), res.size())
+            (res.start(), res.size())
         };
 
-        let release_fn = if flags.contains(Flags::IORESOURCE_MEM) {
-            bindings::release_mem_region
-        } else {
-            bindings::release_region
-        };
-
-        // SAFETY: Safe as per the invariant of `Region`.
-        unsafe { release_fn(start, size) };
+        // SAFETY:
+        // - `parent` is the resource passed to `__request_region`.
+        // - Its lifetime is tied to this `Region`, so it remains valid.
+        // - `start` and `size` identify the region owned by `self`.
+        unsafe { bindings::__release_region(self.parent.0.get(), start, size) };
     }
 }
 
 // SAFETY: `Region` only holds a pointer to a C `struct resource`, which is safe to be used from
 // any thread.
-unsafe impl Send for Region {}
+unsafe impl Send for Region<'_> {}
 
 // SAFETY: `Region` only holds a pointer to a C `struct resource`, references to which are
 // safe to be used from any thread.
-unsafe impl Sync for Region {}
+unsafe impl Sync for Region<'_> {}
 
 /// A resource abstraction.
 ///
@@ -98,13 +99,13 @@ impl Resource {
     /// Exclusive access will be given and the region will be marked as busy.
     /// Further calls to [`Self::request_region`] will return [`None`] if
     /// the region, or a part of it, is already in use.
-    pub fn request_region(
-        &self,
+    pub fn request_region<'a>(
+        &'a self,
         start: PhysAddr,
         size: ResourceSize,
         name: CString,
         flags: Flags,
-    ) -> Option<Region> {
+    ) -> Option<Region<'a>> {
         // SAFETY:
         // - Safe as per the invariant of `Resource`.
         // - `__request_region` will store a reference to the name, but that is
@@ -122,6 +123,7 @@ pub fn request_region(
 
         Some(Region {
             resource: NonNull::new(region)?,
+            parent: self,
             _name: name,
         })
     }
-- 
2.34.1