Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu, Jun 28, 2018 at 01:42:52AM +0200, Ard Biesheuvel wrote: > But what point is there to letting LSMs decide that they do not trust > an I/O device if there is nothing we can do about it? How can we > prevent such an I/O device from modifying our memory? Simply LSMs can opt to not trust such setup. Its their choice. The solution to addressing the concern is orthogonal to their choice. Luis ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 28 June 2018 at 01:33, Luis R. Rodriguez wrote: > On Thu, Jun 28, 2018 at 12:21:47AM +0200, Ard Biesheuvel wrote: >> On 27 June 2018 at 20:00, Luis R. Rodriguez 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 >> >> > 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. > No, memremap() and ioremap() give the *CPU* access to memory. Other bus masters can freely access memory [unless they are behind an IOMMU] >> 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. > But what point is there to letting LSMs decide that they do not trust an I/O device if there is nothing we can do about it? How can we prevent such an I/O device from modifying our memory? >> 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
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu, Jun 28, 2018 at 12:21:47AM +0200, Ard Biesheuvel wrote: > On 27 June 2018 at 20:00, Luis R. Rodriguez 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 > >> > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 27 June 2018 at 20:00, Luis R. Rodriguez 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 >> > 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 de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
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 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. > 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 de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu 07 Jun 11:42 PDT 2018, Ard Biesheuvel wrote: > On 7 June 2018 at 20:21, Bjorn Andersson 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, 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. Regards, Bjorn ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu, Jun 07, 2018 at 11:06:11AM -0700, Bjorn Andersson wrote: > On Thu 07 Jun 09:23 PDT 2018, Ard Biesheuvel wrote: > > > On 7 June 2018 at 18:18, Bjorn Andersson wrote: > > > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: > > > > > >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > > >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... > > So it's this memremap() region that we pass to > request_firmware_into_buf() currently, the previously mentioned > dma_alloc_coherent() region is used as we invoke the secure world > operation to set up the firmware authentication. memremap() is for IO memory, and in that sense it is also not much different from DMA memory in terms of the concerns Mimi has for LSMs and what guarantees LSMs can make to users. Regardless of the device, once you write certain data to the IO memory we cannot be sure the device will wait for all IO to be written, this will be device specific. As such I would suggest READING_IOMEM for this case have request_firmware_into_buf() use it. With that said, since we have only one user of this caller, a future rename to reflect its current actual use would be good. The rename can wait though. Luis ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 06/07/2018 06:33 PM, Greg Kroah-Hartman wrote: > On Thu, Jun 07, 2018 at 06:23:01PM +0200, Ard Biesheuvel wrote: >> On 7 June 2018 at 18:18, Bjorn Andersson wrote: >>> On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: >>> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > On Tue, May 08, 2018 at 03:38:05PM +, 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... >>> >>> The patch at the top of this thread doesn't interest me and you didn't >>> bother sending your question To me. >>> >>> As a matter of fact I'm confused to what the actual question is. >>> >> >> The actual question is whether it is really required that the firmware >> is loaded by the kernel into a buffer that is already mapped for DMA >> at that point, and thus accessible by the device. >> >> To me, it is not entirely clear what the nature is of the firmware >> that we are talking about, since it seems to be getting passed to the >> secure world as well? >> >> In any case, the preferred model in terms of validation/sig checking is >> >> 1) allocate a CPU accessible buffer >> >> 2) request the firmware into it (which may include a sig check under the >> hood) >> >> 3) map the buffer for DMA to the device so it can load the firmware. >> >> 4) kick off the DMA transfer. >> >> The use of dma_alloc_coherent() for this purpose seems unnecessary, >> given that the DMA transfer is not bidirectional. Would it be possible >> to replace it with something like the above sequence? > > Why not just use kmalloc, it will always return a DMAable buffer. DMAble in what sense? For devices that can't handle physical addresses above 16M you need to pass __GFP_DMA to get those, from ZONE_DMA. Otherwise it can return anything from lowmem. That's for x86_64, some other arches have different DMA zone. > Is the problem that vmalloc() might not? vmalloc() could only be used as an alternative if you used kvmalloc(), otherwise kmalloc() won't give you anything from vmalloc > We need to drop the whole DMA zone crud, it confuses everyone who sees > it and was primarily for really really old systems. Yeah that would be nice. > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 7 June 2018 at 20:21, Bjorn Andersson wrote: > On Thu 07 Jun 09:33 PDT 2018, Greg Kroah-Hartman wrote: > >> On Thu, Jun 07, 2018 at 06:23:01PM +0200, Ard Biesheuvel wrote: >> > On 7 June 2018 at 18:18, Bjorn Andersson >> > wrote: >> > > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: >> > > >> > >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: >> > >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... >> > >> >> > > >> > > The patch at the top of this thread doesn't interest me and you didn't >> > > bother sending your question To me. >> > > >> > > As a matter of fact I'm confused to what the actual question is. >> > > >> > >> > The actual question is whether it is really required that the firmware >> > is loaded by the kernel into a buffer that is already mapped for DMA >> > at that point, and thus accessible by the device. >> > >> > To me, it is not entirely clear what the nature is of the firmware >> > that we are talking about, since it seems to be getting passed to the >> > secure world as well? >> > >> > In any case, the preferred model in terms of validation/sig checking is >> > >> > 1) allocate a CPU accessible buffer >> > >> > 2) request the firmware into it (which may include a sig check under the >> > hood) >> > >> > 3) map the buffer for DMA to the device so it can load the firmware. >> > >> > 4) kick off the DMA transfer. >> > >> > The use of dma_alloc_coherent() for this purpose seems unnecessary, >> > given that the DMA transfer is not bidirectional. Would it be possible >> > to replace it with something like the above sequence? >> >> 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. > For this a way to allocate a chunk of physical memory dynamically and > then unmapping and remapping it dynamically in Linux sounds like a > solution, instead of (ab)using the DMA API. This could also serve as > basis for the firmware memory, where firmware is position independent - > in which case this would be passed to request_firmware_into_buf(). > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu 07 Jun 09:33 PDT 2018, Greg Kroah-Hartman wrote: > On Thu, Jun 07, 2018 at 06:23:01PM +0200, Ard Biesheuvel wrote: > > On 7 June 2018 at 18:18, Bjorn Andersson wrote: > > > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: > > > > > >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > > >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... > > >> > > > > > > The patch at the top of this thread doesn't interest me and you didn't > > > bother sending your question To me. > > > > > > As a matter of fact I'm confused to what the actual question is. > > > > > > > The actual question is whether it is really required that the firmware > > is loaded by the kernel into a buffer that is already mapped for DMA > > at that point, and thus accessible by the device. > > > > To me, it is not entirely clear what the nature is of the firmware > > that we are talking about, since it seems to be getting passed to the > > secure world as well? > > > > In any case, the preferred model in terms of validation/sig checking is > > > > 1) allocate a CPU accessible buffer > > > > 2) request the firmware into it (which may include a sig check under the > > hood) > > > > 3) map the buffer for DMA to the device so it can load the firmware. > > > > 4) kick off the DMA transfer. > > > > The use of dma_alloc_coherent() for this purpose seems unnecessary, > > given that the DMA transfer is not bidirectional. Would it be possible > > to replace it with something like the above sequence? > > 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. For this a way to allocate a chunk of physical memory dynamically and then unmapping and remapping it dynamically in Linux sounds like a solution, instead of (ab)using the DMA API. This could also serve as basis for the firmware memory, where firmware is position independent - in which case this would be passed to request_firmware_into_buf(). Regards, Bjorn ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu 07 Jun 09:23 PDT 2018, Ard Biesheuvel wrote: > On 7 June 2018 at 18:18, Bjorn Andersson wrote: > > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: > > > >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... > >> > > > > The patch at the top of this thread doesn't interest me and you didn't > > bother sending your question To me. > > > > As a matter of fact I'm confused to what the actual question is. > > > Thanks Ard, for filling in. > The actual question is whether it is really required that the firmware > is loaded by the kernel into a buffer that is already mapped for DMA > at that point, and thus accessible by the device. > "The device" here refers to additional CPUs found in the Qualcomm SoCs, which executes firmware from the system's DDR memory. > To me, it is not entirely clear what the nature is of the firmware > that we are talking about, since it seems to be getting passed to the > secure world as well? > > In any case, the preferred model in terms of validation/sig checking is > > 1) allocate a CPU accessible buffer > > 2) request the firmware into it (which may include a sig check under the hood) > > 3) map the buffer for DMA to the device so it can load the firmware. > > 4) kick off the DMA transfer. > I think these steps would relate to devices where we load firmware into the device. Here we're loading the firmware into DDR, setting up memory protection (locking out Linux), verifying the firmware and booting the CPU off the loaded and verified firmware. > The use of dma_alloc_coherent() for this purpose seems unnecessary, > given that the DMA transfer is not bidirectional. Would it be possible > to replace it with something like the above sequence? > The majority of these firmwares are position dependent, so we need to use reserved-memory carveouts to position these. The prior art of allocating this memory was dma_alloc_coherent(), but as this has size limitations we currently use memremap() to map these memory regions. There are some firmware that are position independent, so allocating the memory for these dynamically would be preferred, but as the any accesses to this memory region while the device is running would cause access violations we've been using dma_alloc_coherent(). (Although I think we've now reverted to using reserved-memory and memremap for these as well, as Arnd requested that we don't pass the dma_addr_t to our secure world firmware authenticator - i.e. we have no way of benefiting from CMA). So it's this memremap() region that we pass to request_firmware_into_buf() currently, the previously mentioned dma_alloc_coherent() region is used as we invoke the secure world operation to set up the firmware authentication. Regards, Bjorn ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 7 June 2018 at 18:49, Greg Kroah-Hartman wrote: > On Thu, Jun 07, 2018 at 06:43:05PM +0200, Ard Biesheuvel wrote: >> On 7 June 2018 at 18:33, Greg Kroah-Hartman >> wrote: >> > On Thu, Jun 07, 2018 at 06:23:01PM +0200, Ard Biesheuvel wrote: >> >> On 7 June 2018 at 18:18, Bjorn Andersson >> >> wrote: >> >> > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: >> >> > >> >> >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: >> >> >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... >> >> >> >> >> > >> >> > The patch at the top of this thread doesn't interest me and you didn't >> >> > bother sending your question To me. >> >> > >> >> > As a matter of fact I'm confused to what the actual question is. >> >> > >> >> >> >> The actual question is whether it is really required that the firmware >> >> is loaded by the kernel into a buffer that is already mapped for DMA >> >> at that point, and thus accessible by the device. >> >> >> >> To me, it is not entirely clear what the nature is of the firmware >> >> that we are talking about, since it seems to be getting passed to the >> >> secure world as well? >> >> >> >> In any case, the preferred model in terms of validation/sig checking is >> >> >> >> 1) allocate a CPU accessible buffer >> >> >> >> 2) request the firmware into it (which may include a sig check under the >> >> hood) >> >> >> >> 3) map the buffer for DMA to the device so it can load the firmware. >> >> >> >> 4) kick off the DMA transfer. >> >> >> >> The use of dma_alloc_coherent() for this purpose seems unnecessary, >> >> given that the DMA transfer is not bidirectional. Would it be possible >> >> to replace it with something like the above sequence? >> > >> > Why not just use kmalloc, it will always return a DMAable buffer. >> > >> >> On a PC maybe. But there are plenty of systems where bidirectional DMA >> mappings require uncached memory (i.e., if the device doesn't snoop >> the caches), in which case a kmalloc'ed buffer is useless. >> dma_alloc_coherent() hides the platform constraints from the driver, >> so it is a very useful abstraction for this use case. > > kmalloc should always return a DMAble buffer. If that is not true, we > have a _LOT_ of broken drivers. What systems is this not true on, and > how are you running USB on them? :) > Non-cache coherent EHCI and XHCI work absolutely fine in Linux. The driver stack is perfectly well behaved, in the sense that it uses dma_alloc_coherent() for the data structures that are shared between the CPUs and the host controller. For the actual data that gets passed over USB, streaming DMA is used rather than coherent aka consistent aka bidirectional DMA, and that works fine with kmalloc'ed buffers, since you can use bounce buffering if the memory is not accessible to the device directly. That also means that you may prefer to allocate from a special DMA zone to prevent his, i.e., to ensure that the memory passed into the streaming DMA api does not require bounce buffering in the first place. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu, Jun 07, 2018 at 06:43:05PM +0200, Ard Biesheuvel wrote: > On 7 June 2018 at 18:33, Greg Kroah-Hartman > wrote: > > On Thu, Jun 07, 2018 at 06:23:01PM +0200, Ard Biesheuvel wrote: > >> On 7 June 2018 at 18:18, Bjorn Andersson > >> wrote: > >> > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: > >> > > >> >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > >> >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... > >> >> > >> > > >> > The patch at the top of this thread doesn't interest me and you didn't > >> > bother sending your question To me. > >> > > >> > As a matter of fact I'm confused to what the actual question is. > >> > > >> > >> The actual question is whether it is really required that the firmware > >> is loaded by the kernel into a buffer that is already mapped for DMA > >> at that point, and thus accessible by the device. > >> > >> To me, it is not entirely clear what the nature is of the firmware > >> that we are talking about, since it seems to be getting passed to the > >> secure world as well? > >> > >> In any case, the preferred model in terms of validation/sig checking is > >> > >> 1) allocate a CPU accessible buffer > >> > >> 2) request the firmware into it (which may include a sig check under the > >> hood) > >> > >> 3) map the buffer for DMA to the device so it can load the firmware. > >> > >> 4) kick off the DMA transfer. > >> > >> The use of dma_alloc_coherent() for this purpose seems unnecessary, > >> given that the DMA transfer is not bidirectional. Would it be possible > >> to replace it with something like the above sequence? > > > > Why not just use kmalloc, it will always return a DMAable buffer. > > > > On a PC maybe. But there are plenty of systems where bidirectional DMA > mappings require uncached memory (i.e., if the device doesn't snoop > the caches), in which case a kmalloc'ed buffer is useless. > dma_alloc_coherent() hides the platform constraints from the driver, > so it is a very useful abstraction for this use case. kmalloc should always return a DMAble buffer. If that is not true, we have a _LOT_ of broken drivers. What systems is this not true on, and how are you running USB on them? :) thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 7 June 2018 at 18:33, Greg Kroah-Hartman wrote: > On Thu, Jun 07, 2018 at 06:23:01PM +0200, Ard Biesheuvel wrote: >> On 7 June 2018 at 18:18, Bjorn Andersson wrote: >> > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: >> > >> >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: >> >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... >> >> >> > >> > The patch at the top of this thread doesn't interest me and you didn't >> > bother sending your question To me. >> > >> > As a matter of fact I'm confused to what the actual question is. >> > >> >> The actual question is whether it is really required that the firmware >> is loaded by the kernel into a buffer that is already mapped for DMA >> at that point, and thus accessible by the device. >> >> To me, it is not entirely clear what the nature is of the firmware >> that we are talking about, since it seems to be getting passed to the >> secure world as well? >> >> In any case, the preferred model in terms of validation/sig checking is >> >> 1) allocate a CPU accessible buffer >> >> 2) request the firmware into it (which may include a sig check under the >> hood) >> >> 3) map the buffer for DMA to the device so it can load the firmware. >> >> 4) kick off the DMA transfer. >> >> The use of dma_alloc_coherent() for this purpose seems unnecessary, >> given that the DMA transfer is not bidirectional. Would it be possible >> to replace it with something like the above sequence? > > Why not just use kmalloc, it will always return a DMAable buffer. > On a PC maybe. But there are plenty of systems where bidirectional DMA mappings require uncached memory (i.e., if the device doesn't snoop the caches), in which case a kmalloc'ed buffer is useless. dma_alloc_coherent() hides the platform constraints from the driver, so it is a very useful abstraction for this use case. > Is the problem that vmalloc() might not? > > We need to drop the whole DMA zone crud, it confuses everyone who sees > it and was primarily for really really old systems. > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu, Jun 07, 2018 at 06:23:01PM +0200, Ard Biesheuvel wrote: > On 7 June 2018 at 18:18, Bjorn Andersson wrote: > > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: > > > >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... > >> > > > > The patch at the top of this thread doesn't interest me and you didn't > > bother sending your question To me. > > > > As a matter of fact I'm confused to what the actual question is. > > > > The actual question is whether it is really required that the firmware > is loaded by the kernel into a buffer that is already mapped for DMA > at that point, and thus accessible by the device. > > To me, it is not entirely clear what the nature is of the firmware > that we are talking about, since it seems to be getting passed to the > secure world as well? > > In any case, the preferred model in terms of validation/sig checking is > > 1) allocate a CPU accessible buffer > > 2) request the firmware into it (which may include a sig check under the hood) > > 3) map the buffer for DMA to the device so it can load the firmware. > > 4) kick off the DMA transfer. > > The use of dma_alloc_coherent() for this purpose seems unnecessary, > given that the DMA transfer is not bidirectional. Would it be possible > to replace it with something like the above sequence? Why not just use kmalloc, it will always return a DMAable buffer. Is the problem that vmalloc() might not? We need to drop the whole DMA zone crud, it confuses everyone who sees it and was primarily for really really old systems. greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: > On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > > On Tue, May 08, 2018 at 03:38:05PM +, 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... > The patch at the top of this thread doesn't interest me and you didn't bother sending your question To me. As a matter of fact I'm confused to what the actual question is. > 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! > Where do you want to put this? > 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? > >From what I can see [0] says is to use READING_FIRMWARE_PREALLOC_BUFFER regardless of where the memory comes from. Regards, Bjorn > [0] https://patchwork.kernel.org/patch/9074611/ > > Luis ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 7 June 2018 at 18:18, Bjorn Andersson wrote: > On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote: > >> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: >> > On Tue, May 08, 2018 at 03:38:05PM +, 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... >> > > The patch at the top of this thread doesn't interest me and you didn't > bother sending your question To me. > > As a matter of fact I'm confused to what the actual question is. > The actual question is whether it is really required that the firmware is loaded by the kernel into a buffer that is already mapped for DMA at that point, and thus accessible by the device. To me, it is not entirely clear what the nature is of the firmware that we are talking about, since it seems to be getting passed to the secure world as well? In any case, the preferred model in terms of validation/sig checking is 1) allocate a CPU accessible buffer 2) request the firmware into it (which may include a sig check under the hood) 3) map the buffer for DMA to the device so it can load the firmware. 4) kick off the DMA transfer. The use of dma_alloc_coherent() for this purpose seems unnecessary, given that the DMA transfer is not bidirectional. Would it be possible to replace it with something like the above sequence? -- Ard. >> 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! >> > > Where do you want to put this? > >> 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? >> > > From what I can see [0] says is to use READING_FIRMWARE_PREALLOC_BUFFER > regardless of where the memory comes from. > > Regards, > Bjorn > >> [0] https://patchwork.kernel.org/patch/9074611/ >> >> Luis ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Thu, Jun 07, 2018 at 12:41:12AM +0200, Ard Biesheuvel wrote: > On 7 June 2018 at 00:29, Luis R. Rodriguez wrote: > > 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. Given 1 month and no response I don't think we can count on that at this point. Luis -- Do not panic ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 7 June 2018 at 00:29, Luis R. Rodriguez wrote: > On Wed, Jun 06, 2018 at 10:41:07PM +0200, Ard Biesheuvel wrote: >> On 6 June 2018 at 22:32, Luis R. Rodriguez wrote: >> > On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: >> >> On Tue, May 08, 2018 at 03:38:05PM +, 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.gj4...@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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Wed, Jun 06, 2018 at 10:41:07PM +0200, Ard Biesheuvel wrote: > On 6 June 2018 at 22:32, Luis R. Rodriguez wrote: > > On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > >> On Tue, May 08, 2018 at 03:38:05PM +, 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.gj4...@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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On 6 June 2018 at 22:32, Luis R. Rodriguez wrote: > On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: >> On Tue, May 08, 2018 at 03:38:05PM +, 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. 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. 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) 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. > 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/ > > Luis ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()?
On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote: > On Tue, May 08, 2018 at 03:38:05PM +, 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! 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/ Luis ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
