linux-kernel - Re: [PATCH] spi: Ensure memory used for spi_write_then

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: <Z917hRQM2ZhSwvFx@finisterre.sirena.org.uk>
Date: Fri, 21 Mar 2025 14:45:25 +0000
From: Mark Brown <broonie@...nel.org>
To: Arnd Bergmann <arnd@...db.de>
Cc: Petr Tesarik <ptesarik@...e.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org
Subject: Re: [PATCH] spi: Ensure memory used for spi_write_then_read() is DMA
 safe

On Fri, Mar 21, 2025 at 01:41:52PM +0100, Arnd Bergmann wrote:
> On Thu, Mar 20, 2025, at 19:39, Mark Brown wrote:

> > Yes, like I said elsewhere in the thread 16 is a popular number but I
> > suspect that was the thining there.

> Ok, so do we assume we don't need the GFP_DMA then? That's
> fine with me, but in that case we should probably enable
> swiotlb on all arm32 platforms that may have ZONE_DMA smaller
> than ZONE_NORMAL.

I think that makes sense.

> >> - the way that spi_map_buf_attrs() is written, it actually
> >>   supports addresses from both kmap() and vmalloc() and
> >>   will attempt to correctly map those rather than reject
> >>   the buffer. While this sounds like a good idea, handling
> >>   vmalloc data like this risks stack data corruption
> >>   on non-coherent platforms when failing to map stack
> >>   buffers would be the better response.

> > IIRC that's there to support filesystems on SPI flashes or some other
> > application that uses vmalloc()ed buffers, it's definitely not intended
> > to support data on stack.  If it does anything for stack allocated data
> > that's accidental.

> Ok, then the question is what we should do about callers that pass
> in stack data. I can send a patch that adds a WARN_ONCE() or similar,
> but it would trigger on things like 

... (single/double register raw I/O from stack) ...

> which happens in a number of drivers but is harmless as long
> as the driver doesn't actually try to DMA into that buffer.

Hrm, right.  I think that usage is reasonable so we probably should
allow it rather than forcing things to do an allocation for a transfer
that ends up being PIOed anyway almost all the time, OTOH the same API
is also used for large transfers like firmware downloads where we don't
want to trigger a spurious copy.  Having different requirements at
different times would be miserable though so I think we need to just
accept any buffer and then figure it out inside the API, but I've not
fully thought that through yet.

> >> __spi_map_msg() already handles the case of an external
> >> DMA master through ctlr->dma_map_dev, so I think the same
> >> could be used to get a temporary buffer using
> >> dma_alloc_noncoherent() inside of spi_write_then_read()
> >> in place of the kmalloc(, GFP_DMA).

> > That only helps spi_write_then_read() though.

> Right, we'd need to mirror this in other interfaces, either changing
> the existing ones, or adding safer ones that can be used from regmap
> and from drivers that currently allocate their own GFP_DMA buffers
> for this purpose.

Yes, indeed.  I don't have a clear sense for what the best solution is
there yet.  Possibly some libary code for the "I want to DMA this random
memory" use case?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)