linux-kernel - Re: [PATCH v7 5/6] rust: pci: add config space read/write support

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: <DEHUBNKNSNXH.14A4OGQKY5KZR@nvidia.com>
Date: Tue, 25 Nov 2025 23:20:33 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Cc: <dakr@...nel.org>, <aliceryhl@...gle.com>, <bhelgaas@...gle.com>,
 <kwilczynski@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <lossin@...nel.org>, <a.hindborg@...nel.org>, <tmgross@...ch.edu>,
 <markus.probst@...teo.de>, <helgaas@...nel.org>, <cjia@...dia.com>,
 <smitra@...dia.com>, <ankita@...dia.com>, <aniketa@...dia.com>,
 <kwankhede@...dia.com>, <targupta@...dia.com>, <acourbot@...dia.com>,
 <joelagnelf@...dia.com>, <jhubbard@...dia.com>, <zhiwang@...nel.org>
Subject: Re: [PATCH v7 5/6] rust: pci: add config space read/write support

On Wed Nov 19, 2025 at 8:21 PM JST, Zhi Wang wrote:
> Drivers might need to access PCI config space for querying capability
> structures and access the registers inside the structures.
>
> For Rust drivers need to access PCI config space, the Rust PCI abstraction
> needs to support it in a way that upholds Rust's safety principles.
>
> Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
> accessors for PCI config space. The new type implements the `Io` trait to
> share offset validation and bound-checking logic with others.
>
> Cc: Alexandre Courbot <acourbot@...dia.com>
> Cc: Danilo Krummrich <dakr@...nel.org>
> Cc: Joel Fernandes <joelagnelf@...dia.com>
> Signed-off-by: Zhi Wang <zhiw@...dia.com>
> ---
>  rust/kernel/pci.rs    |  43 ++++++++++++++-
>  rust/kernel/pci/io.rs | 118 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 159 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 82e128431f08..f373413e8a84 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -40,7 +40,10 @@
>      ClassMask,
>      Vendor, //
>  };
> -pub use self::io::Bar;
> +pub use self::io::{
> +    Bar,
> +    ConfigSpace, //
> +};
>  pub use self::irq::{
>      IrqType,
>      IrqTypes,
> @@ -331,6 +334,30 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>      }
>  }
>  
> +/// Represents the size of a PCI configuration space.
> +///
> +/// PCI devices can have either a *normal* (legacy) configuration space of 256 bytes,
> +/// or an *extended* configuration space of 4096 bytes as defined in the PCI Express
> +/// specification.
> +#[repr(usize)]
> +pub enum ConfigSpaceSize {
> +    /// 256-byte legacy PCI configuration space.
> +    Normal = 256,
> +
> +    /// 4096-byte PCIe extended configuration space.
> +    Extended = 4096,
> +}
> +
> +impl ConfigSpaceSize {
> +    /// Get the raw value of this enum.
> +    #[inline(always)]
> +    pub const fn as_raw(self) -> usize {
> +        // CAST: PCI configuration space size is at most 4096 bytes, so the value always fits
> +        // within `usize` without truncation or sign change.
> +        self as usize

While correct, an even more solid guarantee is the fact that `self` is
itself represented by a `usize`.

> +    }
> +}
> +
>  impl Device {
>      /// Returns the PCI vendor ID as [`Vendor`].
>      ///
> @@ -427,6 +454,20 @@ pub fn pci_class(&self) -> Class {
>          // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>          Class::from_raw(unsafe { (*self.as_raw()).class })
>      }
> +
> +    /// Returns the size of configuration space.
> +    fn cfg_size(&self) -> Result<ConfigSpaceSize> {
> +        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> +        let size = unsafe { (*self.as_raw()).cfg_size };
> +        match size {
> +            256 => Ok(ConfigSpaceSize::Normal),
> +            4096 => Ok(ConfigSpaceSize::Extended),
> +            _ => {
> +                debug_assert!(false);
> +                Err(EINVAL)
> +            }

Should we implement `TryFrom` for `ConfigSpaceSize` and use it here?

(can't wait for the `TryFrom` derive macro to come and remove the need
for this kind of boilerplate!)

<snip>
> @@ -141,4 +243,18 @@ pub fn iomap_region<'a>(
>      ) -> impl PinInit<Devres<Bar>, Error> + 'a {
>          self.iomap_region_sized::<0>(bar, name)
>      }
> +
> +    /// Return an initialized config space object.
> +    pub fn config_space<'a>(
> +        &'a self,
> +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw() }>> {
> +        Ok(ConfigSpace { pdev: self })
> +    }
> +
> +    /// Return an initialized config space object.
> +    pub fn config_space_extended<'a>(
> +        &'a self,
> +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
> +        Ok(ConfigSpace { pdev: self })
> +    }
>  }

This still has the problem that one can create an extended config space
object even if the device's `cfg_size` is `256`. Both
`config_space` and `config_space_extended` should fail if `cfg_size`
returns an invalid value, and `config_space_extended` should also fail
if the returned size less than `ConfigSpaceSize::Extended`.