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

[Danilo Krummrich <dakr@kernel.org>]

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.