Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?

[Ard Biesheuvel]

On 7 June 2018 at 00:29, Luis R. Rodriguez <mcgrof at kernel.org> wrote:
> 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.
>

True.

> [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.
>

Do you mean 'device' rather than 'CPU' here? The CPU always has access
to memory allocation, but the device generally can only access the
buffer after it has been mapped.

Do note that especially in pc-centric code (which uses cache coherent
DMA that is mapped 1:1 between the CPU physical address space and the
DMA address space), you can actually get away with ignoring map/unmap
entirely, in which case this becomes a 'should' as well.

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

I seriously wonder whether the QCOM code cannot switch to the
streaming API instead. That is generally preferred anyway (for
performance, although that should not matter for loading firmware) but
also removes this single wart for which we have to invent new flags
and new security code plus the associated validation overhead.