Re: slub - extended kmalloc redzone and dma alignment

[Petr Tesarik <ptesarik@suse.com>]

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. ;-)

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().

Unless you are aware of such combination, I suggest to kill the
parameter and implement the "anything else" case above for all
architectures. 

Petr T