linux-kernel - Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bjuil15w.fsf@kernel.org>
Date: Mon, 03 Mar 2025 14:00:11 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "Benno Lossin" <benno.lossin@...ton.me>,  "Abdiel Janulgue"
 <abdiel.janulgue@...il.com>,  <aliceryhl@...gle.com>,  <dakr@...nel.org>,
  <robin.murphy@....com>,  <rust-for-linux@...r.kernel.org>,  "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>,  "Trevor Gross" <tmgross@...ch.edu>,
  "Valentin Obst" <kernel@...entinobst.de>,
  <linux-kernel@...r.kernel.org>,  "Christoph Hellwig" <hch@....de>,
  "Marek Szyprowski" <m.szyprowski@...sung.com>,  <airlied@...hat.com>,
  <iommu@...ts.linux.dev>
Subject: Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

"Daniel Almeida" <daniel.almeida@...labora.com> writes:

> Hi Benno,
>

[...]

>>> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
>>> +    /// number of bytes.
>>> +    ///
>>> +    /// # Examples
>>> +    ///
>>> +    /// ```
>>> +    /// # fn test(alloc: &mut kernel::dma::CoherentAllocation<u8>) -> Result {
>>> +    /// let somedata: [u8; 4] = [0xf; 4];
>>> +    /// let buf: &[u8] = &somedata;
>>> +    /// alloc.write(buf, 0)?;
>>> +    /// # Ok::<(), Error>(()) }
>>> +    /// ```
>>> +    pub fn write(&self, src: &[T], offset: usize) -> Result {
>>> +        let end = offset.checked_add(src.len()).ok_or(EOVERFLOW)?;
>>> +        if end >= self.count {
>>> +            return Err(EINVAL);
>>> +        }
>>> +        // SAFETY:
>>> +        // - The pointer is valid due to type invariant on `CoherentAllocation`
>>> +        // and we've just checked that the range and index is within bounds.
>>> +        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
>>> +        // that `self.count` won't overflow early in the constructor.
>>> +        unsafe {
>>> +            core::ptr::copy_nonoverlapping(src.as_ptr(), self.cpu_addr.add(offset), src.len())
>>
>> Why are there no concurrent write or read operations on `cpu_addr`?
>
> Sorry, can you rephrase this question?

This write is suffering the same complications as discussed here [1].
There are multiple issues with this implementation.

1) `write` takes a shared reference and thus may be called concurrently.
There is no synchronization, so `copy_nonoverlapping` could be called
concurrently on the same address. The safety requirements for
`copy_nonoverlapping` state that the destination must be valid for
write. Alice claims in [1] that any memory area that experience data
races are not valid for writes. So the safety requirement of
`copy_nonoverlapping` is violated and this call is potential UB.

2) The destination of this write is DMA memory. It could be concurrently
modified by hardware, leading to the same issues as 1). Thus the
function cannot be safe if we cannot guarantee hardware will not write
to the region while this function is executing.

Now, I don't think that these _should_ be issues, but according to our
Rust language experts they _are_.

I really think that copying data through a raw pointer to or from a
place that experiences data races, should _not_ be UB if the data is not
interpreted in any way, other than moving it.

Best regards,
Andreas Hindborg

[1] https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/Interacting.20with.20user.20space.20pages