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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z8tDEU2BKj9F3hZ8@cassiopeiae>
Date: Fri, 7 Mar 2025 20:03:45 +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 Fri, Mar 07, 2025 at 12:57:51PM -0400, Jason Gunthorpe wrote:
> 
> Why are you explaining very simple concepts as though I do not
> understand how RCU or devm works?
> 
> I asked you what you intend to protect with the critical region.

When you ask what the critical region protects, I think that it protects the
resource pointer from changing.

I did not read it as "what's meant to be within the critical region".

> 
> I belive you intend to wrapper every memcpy/etc of the allocated
> coherent memory with a RCU critical section, correct?
> 
> Meaning something like:
> 
>   mem.ptr = dma_alloc_coherent(&handle)
>   make_hw_do_dma(handle)
> 
>   start RCU critical section on mem:
>       copy_to_user(mem.ptr) // Sleeps! Can't do it!
>   dma_free_coherent(mem, handle)
> 
> Right?

Yes, that would indeed be a problem. Thanks for pointing it out.

While we could do an SRCU variant, provide separate try_access() methods, etc.,
I think we should do something more efficient:

There is no reason to revoke the *whole* CoherentAllocation object, but only the
the parts that are critical to actually cleanup on driver unbind, i.e. iommu
mappings, etc.

The actual memory allocation itself is not an issue in terms of living beyond
driver unbind and hence doesn't need to be revoked.

With this, you would not need any critical section to access the
CoherentAllocation's memory from the driver.

> Further, if the critical section ever fails to obtain mem.ptr the
> above code is *BUGGY* because it has left a HW DMA running, UAF'd the
> now free'd buffer *and the driver author cannot fix it*.

I don't think that'd be the case in a Rust driver, your example is in C, and
hence doesn't do the Rust style error and cleanup handling that the
corresponding Rust code would do.

But as mentioned above, putting the whole CoherentAllocation in a Devres
container seems wrong anyways. We need to do it at a finer granularity.