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

[Priya Bala Govindasamy <pgovind2@uci.edu>]

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