linux-kernel - Re: [PATCH v2] rust: platform: add Io 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: <CAH5fLgjnu-Yg5+OJn7-yC3pT8tZFcje=+ct_iKXTggu2poQefA@mail.gmail.com>
Date: Fri, 15 Nov 2024 15:38:52 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
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>, 
	Trevor Gross <tmgross@...ch.edu>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Boris Brezillon <boris.brezillon@...labora.com>, 
	Rob Herring <robh@...nel.org>, Daniel Stone <daniels@...labora.com>
Subject: Re: [PATCH v2] rust: platform: add Io support

On Wed, Nov 13, 2024 at 8:32 PM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> Add support for iomem regions by providing a struct IoMem abstraction
> for the platform bus. This will request a memory region and remap it
> into a kernel virtual address using ioremap(). The underlying reads and
> writes are performed by struct Io, which is itself embedded in Iomem.
> This is the equivalent of pci::Bar for the platform bus.
>
> Memory-mapped I/O devices are common, and this patch offers a way to
> program them in Rust, usually by reading and writing to their
> memory-mapped register set.
>
> Both sized and unsized versions are exposed. Sized allocations use
> `ioremap_resource_sized` and specify their size at compile time. Reading
> and writing to IoMem is infallible in this case and no extra runtime
> checks are performed. Unsized allocations have to check the offset
> against the regions's max length at runtime and so return Result.
>
> Lastly, like pci::Bar, platform::IoMem uses the Devres abstraction to
> revoke access to the region if the device is unbound or the underlying
> resource goes out of scope.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>

> +impl<const SIZE: usize> IoMem<SIZE> {
> +    /// Creates a new `IoMem` instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must make sure that `paddr` is a valid MMIO address.
> +    unsafe fn new(pdev: &Device, num: u32, name: &CStr) -> Result<Self> {

The safety comment seems outdated.

Also, you do have another safety requirement: The caller must ensure
that the IoMem does not outlive the device (most likely by wrapping it
in Devres).

> +        let size = pdev.resource_len(num)?;
> +        if size == 0 {
> +            return Err(ENOMEM);
> +        }
> +
> +        let res = pdev.resource(num)?;

You're still calling `bindings::platform_get_resource` twice :(
First call is `resource_len` that calls `resource`. Then you call
`resource` again.

> +        // SAFETY: `res` is guaranteed to be a valid pointer to a `struct
> +        // resource` as per the semantics of `bindings::platform_get_resource()`
> +        let res_start = unsafe { *res }.start;
> +
> +        // SAFETY:
> +        // - `res_start` is guaranteed to be a valid MMIO address.
> +        // - `size` is known not to be zero at this point.
> +        // - `name` is a valid C string.
> +        let mem_region =
> +            unsafe { bindings::request_mem_region(res_start, size, name.as_char_ptr()) };
> +        if mem_region.is_null() {
> +            return Err(EBUSY);
> +        }
> +
> +        // SAFETY:
> +        // - `res_start` is guaranteed to be a valid MMIO address.
> +        // - `size` is known not to be zero at this point.
> +        let addr = unsafe { bindings::ioremap(res_start, size) };
> +        if addr.is_null() {
> +            // SAFETY:
> +            // - `res_start` is guaranteed to be a valid MMIO address.
> +            // - `size` is the same as the one passed to `request_mem_region`.
> +            unsafe { bindings::release_mem_region(res_start, size) };
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
> +        // size `size`.
> +        let io = unsafe { Io::new(addr as usize, size.try_into()?)? };

What types are involved in this .try_into() integer cast? Why is it necessary?

Alice