Re: [PATCH v4 17/27] rust: pci: make Bar lifetime-parameterized

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] `Bar::into_devres` uses `unsafe { core::mem::transmute(self) }` to artificially erase the local `'a` borrow of the `Device`, which is unsound and causes a Use-After-Free during `Devres` destruction.
- [High] The `name` parameter in `iomap_region` and `iomap_region_sized` lacks a `'static` or device-bound lifetime constraint, allowing temporary strings to be passed and leading to a dangling pointer in the kernel's resource tree.
--

commit 00fb543eb5262d30a44ce3dd81c925a773732bd2
Author: Danilo Krummrich <dakr@kernel.org>

rust: pci: make Bar lifetime-parameterized

This patch parameterizes the lifetime of pci::Bar, tying its mapping
lifetime directly to the device. It changes iomap_region_sized to return
a Bar instead of a Devres, and adds an into_devres method to optionally
register it as a device-managed resource with a static lifetime.

> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
[ ... ]
> @@ -219,11 +214,24 @@ unsafe fn do_release(pdev: &Device, ioptr: usize, num: i32) {
>  
>      fn release(&self) {
>          // SAFETY: The safety requirements are guaranteed by the type invariant of `self.pdev`.
> -        unsafe { Self::do_release(&self.pdev, self.io.addr(), self.num) };
> +        unsafe { Self::do_release(self.pdev, self.io.addr(), self.num) };
> +    }
> +
> +    /// Consume the `Bar` and register it as a device-managed resource.
> +    ///
> +    /// The returned `Devres<Bar<'static, SIZE>>` can outlive the original lifetime `'a`. Access
> +    /// to the BAR is revoked when the device is unbound.
> +    pub fn into_devres(self) -> Result<Devres<Bar<'static, SIZE>>> {
> +        // SAFETY: Casting to `'static` is sound because `Devres` guarantees the `Bar` does not
> +        // actually outlive the device -- access is revoked and the resource is released when the
> +        // device is unbound.
> +        let bar: Bar<'static, SIZE> = unsafe { core::mem::transmute(self) };

[Severity: Critical]
Is it safe to transmute this lifetime to 'static here?

Since Rust drops struct fields in declaration order, and Devres declares its
dev: ARef<Device> field before its inner: Arc<Inner<T>> data wrapper, dropping
a Devres<Bar<'static>> will decrement the Device refcount first.

If this was the last reference, the underlying C struct pci_dev could be
freed. When inner then drops, it triggers Bar::drop, which will access the now
dangling pdev pointer to call pci_release_region().

Could this lead to memory corruption during error paths or unbinding?

> +        let pdev = bar.pdev;
> +        Devres::new(pdev.as_ref(), bar)
>      }
>  }
[ ... ]
> @@ -252,17 +260,13 @@ impl Device<device::Bound> {
>      pub fn iomap_region_sized<'a, const SIZE: usize>(
>          &'a self,
>          bar: u32,
> -        name: &'a CStr,
> -    ) -> impl PinInit<Devres<Bar<SIZE>>, Error> + 'a {
> -        Devres::new(self.as_ref(), Bar::<SIZE>::new(self, bar, name))
> +        name: &CStr,

[Severity: High]
Does removing the 'a lifetime bound from the name parameter allow temporary
strings to be passed?

The underlying C function pci_request_region stores the provided string
pointer directly into the name field of the allocated struct resource
without making a copy.

If a caller passes a dynamically allocated temporary CString that drops after
this function returns, could accessing the kernel's resource tree (for example,
by reading /proc/iomem) dereference a freed string pointer?

> +    ) -> Result<Bar<'a, SIZE>> {
> +        Bar::new(self, bar, name)
>      }
>  
>      /// Maps an entire PCI BAR after performing a region-request on it.
> -    pub fn iomap_region<'a>(
> -        &'a self,
> -        bar: u32,
> -        name: &'a CStr,
> -    ) -> impl PinInit<Devres<Bar>, Error> + 'a {
> +    pub fn iomap_region<'a>(&'a self, bar: u32, name: &CStr) -> Result<Bar<'a>> {

[Severity: High]
This seems to have the same lifetime issue with the name parameter as
iomap_region_sized above.

>          self.iomap_region_sized::<0>(bar, name)
>      }
>

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