Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?

[Luis R. Rodriguez]

On Wed, Jun 06, 2018 at 10:41:07PM +0200, Ard Biesheuvel wrote:
> On 6 June 2018 at 22:32, Luis R. Rodriguez <mcgrof at kernel.org> wrote:
> > On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote:
> >> On Tue, May 08, 2018 at 03:38:05PM +0000, Luis R. Rodriguez wrote:
> >> > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> >> > >
> >> > > I think the Qualcomm folks owning this (Andy, David, Bjorn, already
> >> > > cc'd here) are better suited to answer that question.
> >> >
> >> > Andy, David, Bjorn?
> >>
> >> Andy, David, Bjorn?
> >
> > A month now with no answer...
> >
> > Perhaps someone who has this hardware can find out empirically for us, as
> > follows (mm folks is this right?):
> >
> > page = virt_to_page(address);
> > if (!page)
> >    fail closed...
> > if (page_zone(page) == ZONE_DMA || page_zone(page) == ZONE_DMA32)
> >         this is a DMA buffer
> > else
> >         not DMA!
> >
> 
> As I replied in the other thread, this code makes no sense.

OK thanks. If we can't figure it out in code we will have no option
but to expect the worst, specially considering the silence.

> In general, any address covered by the kernel direct mapping can be
> passed to the streaming DMA api and be mapped for device read xor
> device write.

Right, thanks for the details -- on the other thread [0] you've clarified
that with streaming DMA API the CPU *should not* use the buffer and so
*should *be "safe", however that's still a judgement and design call.

[0] https://lkml.kernel.org/r/20180606220605.GJ4511@wotan.suse.de

> Only DMA mappings that will be accessed randomly by the
> device and the CPU at the same time require the use of
> dma_alloc_coherent(), so it can take special precautions (e.g, create
> an uncached mapping if DMA is non cache coherent)

Right and the qcom drivers *does not* use the streaming DMA API, it uses
use dma_alloc_coherent() which explicitly allows the CPU to *immediately*
have access the buffer. We have no control over the CPU and have no ways
to vet that the data we give is complete and correct.

> The DMA zone thing is primarily about reserving low memory ranges for
> DMA because some hardware may not have sufficient address lines wired
> up to access all of DRAM.

Right.

The point to all this is that it is up to LSMs to decide and trust something,
and in this case, even with the streaming DMA API in mind, it is up to LSMs
to decide. In this case we have only *one* user of request_firmware_into_buf()
and code inspection is finding that very likely the dma_alloc_coherent() calls
on the path is actually using this same coherent DMA buffer for firmware.

> > Note that when request_firmware_into_buf() was being reviewed Mimi had asked back
> > in 2016 [0] that if a DMA buffer was going to be used READING_FIRMWARE_DMA should be
> > used otherwise READING_FIRMWARE_PREALLOC_BUFFER was fine.
> >
> > If it is a DMA buffer *now*, why / how did this change?
> >
> > [0] https://patchwork.kernel.org/patch/9074611/

So it is *specially* very odd and disappointing that even though Mimi
*specifically* asked a long time ago that if a DMA buffer would be used it
should be annotated as such with READING_FIRMWARE_DMA, why the annotation
continued forward with READING_FIRMWARE_PREALLOC_BUFFER instead.

Just as Mimi asked for READING_FIRMWARE_DMA it would seem we could reasonably also
ask now or READING_FIRMWARE_DMA_COHERENT and READING_FIRMWARE_DMA_STREAM and some
LSMs will just reject these calls. We don't need READING_FIRMWARE_DMA_STREAM now
as request_firmware_into_buf() users are using dma_alloc_coherent() so we are
trying to determine if request_firmware_into_buf() should pass:

	READING_FIRMWARE_DMA_COHERENT

Given no one is providing a clear answer, and we cannot easily describe the
buffer at run time we'll just move forward with READING_FIRMWARE_DMA_COHERENT.

  Luis