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: <20250307124809.GL354511@nvidia.com>
Date: Fri, 7 Mar 2025 08:48:09 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
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 Fri, Mar 07, 2025 at 09:50:07AM +0100, Danilo Krummrich wrote:
> > Technically it is unsafe and oopsable to call the allocation API as
> > well on a device that has no driver. This issue is also ignored in
> > these bindings and cannot be solved with revoke.
> 
> This is correct, and I am well aware of it. I brought this up once when working
> on the initial device / driver, devres and I/O abstractions.

Yes it is also incorrect to call any devm function on an unprobed
driver.

> It's on my list to make the creation of the Devres container fallible in this
> aspect, which would prevent this issue.

I expect that will require new locking.

> > The actual critical region extends into the HW itself, it is not
> > simple to model this with a pure SW construct of bracketing some
> > allocation. You need to bracket the *entire lifecycle* of the
> > dma_addr_t that has been returned and passed into HW, until the
> > dma_addr_t is removed from HW.
> 
> Devres callbacks run after remove(). It's the drivers job to stop operating the
> device latest in remove(). Which means that the design is correct.

It could be the drivers job to unmap the dma as well if you take that
logic.

You still didn't answer the question, what is the critical region of
the DevRes for a dma_alloc_coherent() actually going to protect?

You also have to urgently fix the synchronize_rcu() repitition of you
plan to do this.

> Now, you ask for a step further, i.e. make it that we can enforce that a driver
> actually stopped the device in remove().

So where do you draw the line on bugs Rust should prevent and bugs
Rust requires the programmer to fix?

Allow UAF from forgetting to shutdown DMA, but try to mitigate UAF
from failing to call a dma unmap function. It is the *very same*
driver bug: incorrect shutdown of DMA activity.

I said this for MMIO, and I say it more strongly here. The correct
thing is to throw a warning if the driver has malfunctioned and leaked
a DMA Mapping. This indicates a driver bug. Silently fixing the issue
does nothing to help driver writers make correct drivers. It may even
confuse authors as to what their responsiblities are since so much is
handled "magically".

> Having that said, it doesn't need to be an "all or nothing", let's catch the
> ones we can actually catch.

Well, that's refreshing. Maybe it would be nice to have an agreed
binding design policy on what is wortwhile to catch with runtime
overhead and what is not.

Jason