On 27 June 2018 at 20:00, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> 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@xxxxxxxxxx> 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. This is not about the risk of loading a file with an invalid signature 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. 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, 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. >> 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. > > Luis _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
