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: <Z8mlAxsszdOH-ow8@cassiopeiae>
Date: Thu, 6 Mar 2025 14:37:07 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Abdiel Janulgue <abdiel.janulgue@...il.com>, aliceryhl@...gle.com,
	robin.murphy@....com, daniel.almeida@...labora.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>,
	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 v12 2/3] rust: add dma coherent allocator abstraction.

On Wed, Mar 05, 2025 at 01:41:19PM -0400, Jason Gunthorpe wrote:
> On Mon, Feb 24, 2025 at 01:49:06PM +0200, Abdiel Janulgue wrote:
> 
> > +impl<T: AsBytes + FromBytes> Drop for CoherentAllocation<T> {
> > +    fn drop(&mut self) {
> > +        let size = self.count * core::mem::size_of::<T>();
> > +        // SAFETY: the device, cpu address, and the dma handle is valid due to the
> > +        // type invariants on `CoherentAllocation`.
> > +        unsafe {
> > +            bindings::dma_free_attrs(
> > +                self.dev.as_raw(),
> > +                size,
> > +                self.cpu_addr as _,
> > +                self.dma_handle,
> > +                self.dma_attrs.as_raw(),
> > +            )
> 
> I mentioned this in another thread..
> 
> There is an additional C API restriction here that the DMA API
> functions may only be called by a driver after probe() starts and
> before remove() completes. This applies to dma_free_attrs().
> 
> It is not enough that a refcount is held on device.
> 
> Otherwise the kernel may crash as the driver core allows resources
> used by the DMA API to be changed once the driver is removed.
> 
> See the related discussion here, with an example of what the crash can
> look like:
> 
> https://lore.kernel.org/lkml/8067f204-1380-4d37-8ffd-007fc6f26738@kernel.org/T/#m0c7dda0fb5981240879c5ca489176987d688844c
> 
>  > a device with no driver bound should not be passed to the DMA API,
>  > much less a dead device that's already been removed from its parent
>  > bus.

Thanks for bringing this up!

I assume that's because of potential iommu mappings, the memory itself should
not be critical.

> 
> My rust is non-existent, but I did not see anything about this
> point.

Indeed, this needs to be fixed. It means that a CoherentAllocation also needs to
be embedded in a Devres container.

> 
> Also note that any HW configured to do DMA must be halted before the
> free is allowed otherwise it is a UAF bug. It is worth mentioning that
> in the documentation.

Agreed, makes sense to document. For embedding the CoherentAllocation into
Devres this shouldn't be an issue, since a driver must stop operating the device
in remove() by definition.