Re: slub - extended kmalloc redzone and dma alignment

[Catalin Marinas <catalin.marinas@arm.com>]

On Wed, Apr 09, 2025 at 02:18:16PM +0200, Petr Tesarik wrote:
> On Wed, 9 Apr 2025 10:47:36 +0100
> Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Apr 09, 2025 at 11:05:29AM +0200, Petr Tesarik wrote:
> > > On Wed, 9 Apr 2025 10:39:04 +0200
> > > Petr Tesarik <ptesarik@suse.com> wrote:  
> > > > I believe there is potential for a nasty race condition, and maybe even
> > > > info leak. Consider this:
> > > > 
> > > > 1. DMA buffer is allocated by kmalloc(). The memory area previously
> > > >    contained sensitive information, which had been written to main
> > > >    memory.
> > > > 2. The DMA buffer is initialized with zeroes, but this new content
> > > >    stays in a CPU cache (because this is kernel memory with a write
> > > >    behind cache policy).
> > > > 3. DMA is set up, but nothing is written to main memory by the
> > > >    bus-mastering device.
> > > > 4. The CPU cache line is now discarded in arch_sync_dma_for_cpu().
> > > > 
> > > > IIUC the zeroes were never written to main memory, and previous content
> > > > can now be read by the CPU through the DMA buffer.
> > > > 
> > > > I haven't checked if any architecture is affected, but I strongly
> > > > believe that the CPU cache MUST be flushed both before and after the
> > > > DMA transfer. Any architecture which does not do it that way should be
> > > > fixed.
> > > > 
> > > > Or did I miss a crucial detail (again)?  
> > > 
> > > Just after sending this, I realized I did. :(
> > > 
> > > There is a step between 2 and 3:
> > > 
> > > 2a. arch_sync_dma_for_device() invalidates the CPU cache line.
> > >     Architectures which do not write previous content to main memory
> > >     effectively undo the zeroing here.  
> > 
> > Good point, that's a problem on those architectures that invalidate the
> > caches in arch_sync_dma_for_device(). We fixed it for arm64 in 5.19 -
> > c50f11c6196f ("arm64: mm: Don't invalidate FROM_DEVICE buffers at start
> > of DMA transfer") - for the same reasons, information leak.
> > 
> > So we could ignore all those architectures. If they people complain
> > about redzone failures, we can ask them to fix. Well, crude attempt
> > below at fixing those. I skipped powerpc and for arch/arm I only
> > addressed cache-v7. Completely untested.
> > 
> > But I wonder whether it's easier to fix the callers of
> > arch_sync_dma_for_device and always pass DMA_BIDIRECTIONAL for security
> > reasons:
> 
> Then we could remove this parameter as useless. ;-)

Yes, I just took the lazy approach of not changing the arch code.

> Now, arch_sync_for_device() must always do the same thing, regardless
> of the transfer direction: Write back any cached DMA buffer data to
> main memory. Except, if an architectures can do this with or without
> invalidating the cache line _and_ it never loads cache content
> speculatively (does not even have a prefetch instruction), it might
> optimize like this:
> 
> - DMA_FROM_DEVICE:
>   - arch_sync_for_device(): writeback and invalidate
>   - arch_sync_for_cpu(): nothing
> 
> - anythig else:
>   - arch_sync_for_device(): writeback only
>   - arch_sync_for_cpu(): invalidate unless DMA_TO_DEVICE
> 
> I don't know if there's any such non-prefetching architecture, much
> less if this optimization would make sense there. It's just the only
> scenario I can imagine where the direction parameter makes any
> difference for arch_sync_for_device().

We have at least some old arm cores (pre ARMv6) that have a no-op for
the arch_sync_dma_for_cpu() case. They rely on the for_device code to
invalidate the caches. That's why I suggested DMA_BIDIRECTIONAL as a
cover-all option. However, such default may affect the performance of
CPUs that would benefit from a writeback without invalidate.

Basically what we need is something like:

	if (dir == DMA_TO_DEVICE)
		dir = DMA_BIDIRECTIONAL;

We could hide this in the core DMA code (behind some macro/function) or
update the arch code to do this. I don't have the setup to test all of
them (I can test arm64, maybe modern arm32 but that's about it).

Or we ignore the problem and tell people to fix their DMA ops if they
see complains about slab redzones. We also ignore the potential
information leak we've had for decades.

-- 
Catalin