| 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
| ||
|
Message-ID: <Z5D_IxCZvcZ_jKd8@pollux>
Date: Wed, 22 Jan 2025 15:22:27 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Fiona Behrens <me@...enk.dev>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Daniel Almeida <daniel.almeida@...labora.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: io: move offset_valid and io_addr(_assert) to IoRaw
On Wed, Jan 22, 2025 at 01:38:09PM +0100, Fiona Behrens wrote:
> Move the helper functions `offset_valid`, `io_addr` and
> `io_addr_asset` from `Io` to `IoRaw`. This allows `IoRaw` to be reused
> if other abstractions with different write/read functions are
> needed (e.g. `writeb` vs `iowrite` vs `outb`).
>
> Make this functions public as well so they can be used from other
> modules if you aquire a `IoRaw`.
I don't think they should be public. Instead the abstraction for I/O ports
should be in this file, just like `Io` is.
Another option could also be to just extend the existing `Io` abstraction for
I/O ports.
>
> Signed-off-by: Fiona Behrens <me@...enk.dev>
> ---
> rust/kernel/io.rs | 98 +++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 63 insertions(+), 35 deletions(-)
>
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index d4a73e52e3ee68f7b558749ed0108acde92ae5fe..a6d026f458608626113fd194ee5a8616b4ef76fe 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -15,6 +15,11 @@
> /// Instead, the bus specific MMIO implementation must convert this raw representation into an `Io`
> /// instance providing the actual memory accessors. Only by the conversion into an `Io` structure
> /// any guarantees are given.
> +///
> +/// # Invariant
You phrased this invariant as if it would be a requirement, but it's more like a
something that's always uphold. I'd phrase it as a fact that can be relied on.
> +///
> +/// `addr` plus `maxsize` has to fit in memory (smaller than [`usize::MAX`])
"fit in memory" sounds a bit misleading. I think you want to say they have to be
in the range of some address space (e.g. PIO).
Besides that, why do we need this at all in this patch? I think it's fine to
add, but then it should be separate patch I think.
> +/// and `maxsize` has to be smaller or equal to `SIZE`.
That's wrong, it's the other way around.
> pub struct IoRaw<const SIZE: usize = 0> {
> addr: usize,
> maxsize: usize,
> @@ -23,7 +28,7 @@ pub struct IoRaw<const SIZE: usize = 0> {
> impl<const SIZE: usize> IoRaw<SIZE> {
> /// Returns a new `IoRaw` instance on success, an error otherwise.
> pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
> - if maxsize < SIZE {
> + if maxsize < SIZE || addr.checked_add(maxsize).is_none() {
> return Err(EINVAL);
> }
>
> @@ -32,15 +37,66 @@ pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
>
> /// Returns the base address of the MMIO region.
> #[inline]
> - pub fn addr(&self) -> usize {
> + pub const fn addr(&self) -> usize {
> self.addr
> }
>
> /// Returns the maximum size of the MMIO region.
> #[inline]
> - pub fn maxsize(&self) -> usize {
> + pub const fn maxsize(&self) -> usize {
> self.maxsize
> }
> +
> + /// Check if the offset plus the size of the type `U` fits in the bounds of `size`.
> + /// Also checks if the offset is aligned with the type size.
> + #[inline]
> + pub const fn offset_valid<U>(offset: usize, size: usize) -> bool {
> + let type_size = core::mem::size_of::<U>();
> + if let Some(end) = offset.checked_add(type_size) {
> + end <= size && offset % type_size == 0
> + } else {
> + false
> + }
> + }
> +
> + /// Check if the offset (plus the type size) is out of bounds.
> + ///
> + /// Runtime checked version of [`io_addr_assert`].
> + ///
> + /// See [`offset_valid`] for the performed offset check.
> + ///
> + /// # Errors
> + ///
> + /// Returns [`EINVAL`] if the type does not fit into [`IoRaw`] at the given offset.
> + ///
> + /// [`offset_valid`]: Self::offset_valid
> + /// [`io_addr_assert`]: Self::io_addr_assert
> + #[inline]
> + pub fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> + if !Self::offset_valid::<U>(offset, self.maxsize()) {
> + return Err(EINVAL);
> + }
> +
> + // Probably no need to check, since the safety requirements of `Self::new` guarantee that
> + // this can't overflow.
> + self.addr().checked_add(offset).ok_or(EINVAL)
> + }
> +
> + /// Check at build time if the offset (plus the type size) is out of bounds.
> + ///
> + /// Compiletime checked version of [`io_addr`].
> + ///
> + /// See [`offset_valid`] for the performed offset check.
> + ///
> + ///
> + /// [`offset_valid`]: Self::offset_valid
> + /// [`io_addr`]: Self::io_addr
> + #[inline]
> + pub const fn io_addr_assert<U>(&self, offset: usize) -> usize {
> + build_assert!(Self::offset_valid::<U>(offset, SIZE));
> +
> + self.addr() + offset
> + }
> }
>
> /// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
> @@ -116,7 +172,7 @@ macro_rules! define_read {
> $(#[$attr])*
> #[inline]
> pub fn $name(&self, offset: usize) -> $type_name {
> - let addr = self.io_addr_assert::<$type_name>(offset);
> + let addr = self.0.io_addr_assert::<$type_name>(offset);
>
> // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> unsafe { bindings::$name(addr as _) }
> @@ -128,7 +184,7 @@ pub fn $name(&self, offset: usize) -> $type_name {
> /// out of bounds.
> $(#[$attr])*
> pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
> - let addr = self.io_addr::<$type_name>(offset)?;
> + let addr = self.0.io_addr::<$type_name>(offset)?;
>
> // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> Ok(unsafe { bindings::$name(addr as _) })
> @@ -145,7 +201,7 @@ macro_rules! define_write {
> $(#[$attr])*
> #[inline]
> pub fn $name(&self, value: $type_name, offset: usize) {
> - let addr = self.io_addr_assert::<$type_name>(offset);
> + let addr = self.0.io_addr_assert::<$type_name>(offset);
>
> // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> unsafe { bindings::$name(value, addr as _, ) }
> @@ -157,7 +213,7 @@ pub fn $name(&self, value: $type_name, offset: usize) {
> /// out of bounds.
> $(#[$attr])*
> pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
> - let addr = self.io_addr::<$type_name>(offset)?;
> + let addr = self.0.io_addr::<$type_name>(offset)?;
>
> // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> unsafe { bindings::$name(value, addr as _) }
> @@ -190,34 +246,6 @@ pub fn maxsize(&self) -> usize {
> self.0.maxsize()
> }
>
> - #[inline]
> - const fn offset_valid<U>(offset: usize, size: usize) -> bool {
> - let type_size = core::mem::size_of::<U>();
> - if let Some(end) = offset.checked_add(type_size) {
> - end <= size && offset % type_size == 0
> - } else {
> - false
> - }
> - }
> -
> - #[inline]
> - fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> - if !Self::offset_valid::<U>(offset, self.maxsize()) {
> - return Err(EINVAL);
> - }
> -
> - // Probably no need to check, since the safety requirements of `Self::new` guarantee that
> - // this can't overflow.
> - self.addr().checked_add(offset).ok_or(EINVAL)
> - }
> -
> - #[inline]
> - fn io_addr_assert<U>(&self, offset: usize) -> usize {
> - build_assert!(Self::offset_valid::<U>(offset, SIZE));
> -
> - self.addr() + offset
> - }
> -
> define_read!(readb, try_readb, u8);
> define_read!(readw, try_readw, u16);
> define_read!(readl, try_readl, u32);
>
> ---
> base-commit: 01b3cb620815fc3feb90ee117d9445a5b608a9f7
> change-id: 20250122-rust-io-offset-7b39b11e84ac
>
> Best regards,
> --
> Fiona Behrens <me@...enk.dev>
>
Powered by blists - more mailing lists