Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Thu, Jun 28, 2018 at 12:21:47AM +0200, Ard Biesheuvel wrote:
> On 27 June 2018 at 20:00, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Mon, Jun 25, 2018 at 05:08:08PM -0700, Bjorn Andersson wrote:
> >> On Thu 07 Jun 11:42 PDT 2018, Ard Biesheuvel wrote:
> >>
> >> > On 7 June 2018 at 20:21, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> >> > > On Thu 07 Jun 09:33 PDT 2018, Greg Kroah-Hartman wrote:
> >> [..]
> >> > >>
> >> > >> Why not just use kmalloc, it will always return a DMAable buffer.
> >> > >>
> >> > >
> >> > > For the buffers being targeted by request_firmware_into_buf() the
> >> > > problem is that some of them has requirements of physical placement and
> >> > > they are all too big for kmalloc() (i.e. tens of mb).
> >> > >
> >> > >
> >> > > For the dma_alloc_coherent() buffer that was mentioned earlier, which is
> >> > > not related to the firmware loading, it's not used because the buffer is
> >> > > passed to secure world, which temporarily locks Linux out from the
> >> > > memory region. Traditionally this region was kmalloc'ed downstream, but
> >> > > due to speculative access violations this code moved to use the DMA
> >> > > streaming API, although there's no actual DMA going on.
> >> > >
> >> >
> >> > OK, so you are relying on the fact that dma_alloc_coherent() gives you
> >> > a device mapping (because the qcom_scm device is described as non
> >> > cache coherent), but this sounds risky to me. The linear alias of that
> >> > memory will still be mapped cacheable, and could potentially still be
> >> > accessed speculatively AFAIK.
> >> >
> >>
> >> Yes and we are aware of the risk of having the linear alias present, but
> >> have yet to find a suitable way to handle this.
> >>
> >> The proposed mechanism was to use reserved-memory and memremap() the
> >> region while it should be available in Linux,
> >
> > That's still IO memory, and so it would be up to the specific device if
> > or not it could access the memory before a full write is done.
> >
> 
> The risk here is about having aliases with mismatched attributes,
> which may result in loss of coherency and corrupt your data.

That risk is a perhaps a practical risk.

> This is
> not about the risk of loading a file with an invalid signature

This is a theoretical risk LSMs wish to determine and based on information
assess what to do.

> And what do you mean by 'IO memory'? Bus masters that are not behind
> an IOMMU can access all of the kernel's memory all of the time,
> regardless of whether and how it chooses to use it. So from a security
> perspective, there is no distinction, and you can only distinguish
> between before and after informing the device where it can find the
> firmware buffer in memory.

I mean that using memremap() or ioremap() will is designed to give
hardware access to memory for IO purposes, and how writes occur
will vary, and as such we cannot give LSMs guarantees over that
the firmware API will finish a write and that the data provided
really is correct.

> So given that we are dealing here with other masters that can change
> all of your memory behind your back, including the actual code you are
> running that implements the signature check, 

LSMs have the option to trust the kernel, its about context and letting LSMs
decide. They have the right to not trust IO devices to a memory segment, as it
could break guarantees the kernel is making, so this is not about trust or
not, its about *information* and letting LSMs choose.

> I wonder if there is a
> point to obsessing about this use case from a validation point of
> view. The higher privilege level protects itself by doing its own
> signature check, and doing the same at a lower privilege level seems
> redundant to me.

Its up to LSMs to implement the policy.

> >> but while this would work
> >> for some cases (e.g. memory regions for semi-static firmware executed by
> >> co-processors) it doesn't handle the scenarios where the memory-need is
> >> dynamic.
> >>
> >> So suggestions are very welcome on how to better handle this.
> >
> > I *believe* Vlastimil's seems to suggest kvmalloc(), but note that if getting the
> > memory to be contiguous fails, it would fallback to a non-contiguous (vmalloc)
> > allocation.

I gave this some though and this obviously is just as good as trying to just use
kmalloc() as that is what is desired, the issue however *ensuring* that the allocation
will succeed. The only thing that I can think of there is somehow hinting upon boot
to reserve a special allocation for later use. If not at boot, perhaps a hint to
eventually give back the desired contigious allocation, but its beyond me if any
of this is in fact possible.

  Luis