linux-kernel - Re: [RFC PATCH 10/11] rust: add basic abstractions for iomem operations

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: <CE78A4CB-F499-4BD8-8E29-28F561B320C1@redhat.com>
Date: Tue, 21 May 2024 04:59:08 +0200
From: Danilo Krummrich <dakr@...hat.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
 ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
 boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
 benno.lossin@...ton.me, a.hindborg@...sung.com, aliceryhl@...gle.com,
 airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net,
 pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org
Subject: Re: [RFC PATCH 10/11] rust: add basic abstractions for iomem operations

(quick reply from my phone)

> On 21. May 2024, at 00:32, Miguel Ojeda <miguel.ojeda.sandonis@...il.com> wrote:
> 
> On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@...hat.com> wrote:
>> 
>> through its Drop() implementation.
> 
> Nit: `Drop`, `Deref` and so on are traits -- what do the `()` mean
> here? I guess you may be referring to their method, but those are
> lowercase.
> 
>> +/// IO-mapped memory, starting at the base pointer @ioptr and spanning @malxen bytes.
> 
> Please use Markdown code spans instead (and intra-doc links where
> possible) -- we don't use the `@` notation. There is a typo on the
> variable name too.
> 
>> +pub struct IoMem {
>> +    pub ioptr: usize,
> 
> This field is public, which raises some questions...
> 
>> +    pub fn readb(&self, offset: usize) -> Result<u8> {
>> +        let ioptr: usize = self.get_io_addr(offset, 1)?;
>> +
>> +        Ok(unsafe { bindings::readb(ioptr as _) })
>> +    }
> 
> These methods are unsound, since `ioptr` may end up being anything
> here, given `self.ioptr` it is controlled by the caller. One could
> also trigger an overflow in `get_io_addr`.
> 

I think the only thing we really want from IoMem is a generic implementation of the read*() and write*() functions.

Everything else should be up the the resource itself, see e.g. pci::Bar. Hence I think we should just make IoMem a trait instead and just let the resource implement a getter for the MMIO pointer, etc.

> Wedson wrote a similar abstraction in the past
> (`rust/kernel/io_mem.rs` in the old `rust` branch), with a
> compile-time `SIZE` -- it is probably worth taking a look.
> 
> Also, there are missing `// SAFETY:` comments here. Documentation and
> examples would also be nice to have.
> 
> Thanks!
> 
> Cheers,
> Miguel
>