linux-kernel - Re: [PATCH v11 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <cd25d3a6-c203-4460-ab7c-81fa4b56c566@gmail.com>
Date: Thu, 23 Jan 2025 15:38:53 +0200
From: Abdiel Janulgue <abdiel.janulgue@...il.com>
To: Petr Tesařík <petr@...arici.cz>,
 Miguel Ojeda <ojeda@...nel.org>
Cc: rust-for-linux@...r.kernel.org, daniel.almeida@...labora.com,
 dakr@...nel.org, robin.murphy@....com, aliceryhl@...gle.com,
 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>,
 Valentin Obst <kernel@...entinobst.de>,
 open list <linux-kernel@...r.kernel.org>, Christoph Hellwig <hch@....de>,
 Marek Szyprowski <m.szyprowski@...sung.com>, airlied@...hat.com,
 "open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>
Subject: Re: [PATCH v11 2/3] rust: add dma coherent allocator abstraction.

On 23/01/2025 14:30, Petr Tesařík wrote:
> On Thu, 23 Jan 2025 12:42:58 +0200
> Abdiel Janulgue <abdiel.janulgue@...il.com> wrote:
>> +
>> +    /// Reads data from the region starting from `offset` as a slice.
>> +    /// `offset` and `count` are in units of `T`, not the number of bytes.
>> +    ///
>> +    /// Due to the safety requirements of slice, the data returned should be regarded by the
>> +    /// caller as a snapshot of the region when this function is called, as the region could
>> +    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases
>> +    /// where the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()`
>> +    /// could be used instead.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that no hardware operations that involve the buffer are currently
>> +    /// taking place while the returned slice is live.
>> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
>> +        if offset + count >= self.count {
> 
> I'm probably missing something, but how do you know that this addition
> can't overflow? I mean, since this is a public function, users can do
> something dumb such as buf.as_slice(usize::MAX, n), can't they?
> 
> What about something like:
> 
>      let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
>      if end >= self.count { ... }
>

Makes sense. This could also just return EINVAL instead of EOVERFLOW, 
but either way is fine with me.

Miguel, what do you think? Would it be possible just include this change 
and the one below locally if you think this series is ready for merging?

Regards,
Abdiel

>> +            return Err(EINVAL);
>> +        }
>> +        // SAFETY:
>> +        // - The pointer is valid due to type invariant on `CoherentAllocation`,
>> +        // we've just checked that the range and index is within bounds. The immutability of the
>> +        // of data is also guaranteed by the safety requirements of the function.
>> +        // - `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.
>> +        Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.add(offset), count) })
>> +    }
>> +
>> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
>> +    /// number of bytes.
>> +    pub fn write(&self, src: &[T], offset: usize) -> Result {
>> +        if offset + src.len() >= self.count {
> 
> Same here.
> 
> Petr T