This series implements mitigations for lack of DMA access control on
systems without an IOMMU, which could result in the DMA accessing the
system memory at unexpected times and/or unexpected addresses, possibly
leading to data leakage or corruption.
For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is
not behind an IOMMU. As PCI-e, by design, gives the device full access to
system memory, a vulnerability in the Wi-Fi firmware could easily escalate
to a full system exploit (remote wifi exploits: [1a], [1b] that shows a
full chain of exploits; [2], [3]).
To mitigate the security concerns, we introduce restricted DMA. Restricted
DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a
specially allocated region and does memory allocation from the same region.
The feature on its own provides a basic level of protection against the DMA
overwriting buffer contents at unexpected times. However, to protect
against general data leakage and system memory corruption, the system needs
to provide a way to restrict the DMA to a predefined memory region (this is
usually done at firmware level, e.g. in ATF on some ARM platforms).
[1a] https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_4.html
[1b] https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_11.html
[2] https://blade.tencent.com/en/advisories/qualpwn/
[3] https://www.bleepingcomputer.com/news/security/vulnerabilities-found-in-highly-popular-firmware-for-wifi-chips/
Claire Chang (6):
swiotlb: Add io_tlb_mem struct
swiotlb: Add restricted DMA pool
swiotlb: Use restricted DMA pool if available
swiotlb: Add restricted DMA alloc/free support.
dt-bindings: of: Add restricted DMA pool
of: Add plumbing for restricted DMA pool
.../reserved-memory/reserved-memory.txt | 24 +
arch/powerpc/platforms/pseries/svm.c | 4 +-
drivers/iommu/dma-iommu.c | 12 +-
drivers/of/address.c | 21 +
drivers/of/device.c | 4 +
drivers/of/of_private.h | 5 +
drivers/xen/swiotlb-xen.c | 4 +-
include/linux/device.h | 4 +
include/linux/swiotlb.h | 61 +-
kernel/dma/Kconfig | 1 +
kernel/dma/direct.c | 20 +-
kernel/dma/direct.h | 10 +-
kernel/dma/swiotlb.c | 576 +++++++++++-------
13 files changed, 514 insertions(+), 232 deletions(-)
--
2.29.2.729.g45daf8777d-goog
v3:
Using only one reserved memory region for both streaming DMA and memory
allocation.
v2:
Building on top of swiotlb.
https://lore.kernel.org/patchwork/cover/1280705/
v1:
Using dma_map_ops.
https://lore.kernel.org/patchwork/cover/1271660/
@@ -82,6 +82,7 @@ config ARCH_HAS_FORCE_DMA_UNENCRYPTEDconfigSWIOTLBboolselectNEED_DMA_MAP_STATE+selectOF_EARLY_FLATTREE## Should be selected if we can mmap non-coherent mappings to userspace.
Why does this have to be added here? Shouldn't the platform-specific
code handle it instead?
The whole code added here is pretty generic. What we need to eventually
do, though is to add a separate dma_device instead of adding more and more
bloat to struct device.
Why does this have to be added here? Shouldn't the platform-specific
code handle it instead?
The whole code added here is pretty generic. What we need to eventually
do, though is to add a separate dma_device instead of adding more and more
bloat to struct device.
From: Christoph Hellwig <hch@lst.de> Date: 2021-01-13 12:38:25
On Wed, Jan 13, 2021 at 01:29:05PM +0100, Greg KH wrote:
quoted
quoted
Why does this have to be added here? Shouldn't the platform-specific
code handle it instead?
The whole code added here is pretty generic. What we need to eventually
do, though is to add a separate dma_device instead of adding more and more
bloat to struct device.
I have no objections for that happening!
I'm pretty sure you agreed to it before in fact. Now someone just needs
to find the time to do this heavy lifting, where "someone" probably means
me.
The code should be as much as possible arch-agnostic. That is why there
are multiple -swiotlb files scattered in arch directories that own the
architecture specific code.
Would it be possible to move the code there and perhaps have a ARM
specific front-end for this DMA restricted pool there? See for example
the xen-swiotlb code.
Cheers!
Konrad
Hi Greg and Konrad,
This change is intended to be non-arch specific. Any arch that lacks DMA access
control and has devices not behind an IOMMU can make use of it. Could you share
why you think this should be arch specific?
Thanks!
From: Konrad Rzeszutek Wilk <hidden> Date: 2021-01-07 17:59:53
On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote:
Hi Greg and Konrad,
This change is intended to be non-arch specific. Any arch that lacks DMA access
control and has devices not behind an IOMMU can make use of it. Could you share
why you think this should be arch specific?
The idea behind non-arch specific code is it to be generic. The devicetree
is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should
be in arch specific code.
On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote:
quoted
Hi Greg and Konrad,
This change is intended to be non-arch specific. Any arch that lacks DMA access
control and has devices not behind an IOMMU can make use of it. Could you share
why you think this should be arch specific?
The idea behind non-arch specific code is it to be generic. The devicetree
is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should
be in arch specific code.
In premise the same code could be used with an ACPI enabled system with
an appropriate service to identify the restricted DMA regions and unlock
them.
More than 1 architecture requiring this function (ARM and ARM64 are the
two I can think of needing this immediately) sort of calls for making
the code architecture agnostic since past 2, you need something that scales.
There is already code today under kernel/dma/contiguous.c that is only
activated on a CONFIG_OF=y && CONFIG_OF_RESERVED_MEM=y system, this is
no different.
--
Florian
From: Konrad Rzeszutek Wilk <hidden> Date: 2021-01-07 21:24:28
On Thu, Jan 07, 2021 at 10:09:14AM -0800, Florian Fainelli wrote:
On 1/7/21 9:57 AM, Konrad Rzeszutek Wilk wrote:
quoted
On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote:
quoted
Hi Greg and Konrad,
This change is intended to be non-arch specific. Any arch that lacks DMA access
control and has devices not behind an IOMMU can make use of it. Could you share
why you think this should be arch specific?
The idea behind non-arch specific code is it to be generic. The devicetree
is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should
be in arch specific code.
In premise the same code could be used with an ACPI enabled system with
an appropriate service to identify the restricted DMA regions and unlock
them.
Which this patchset is not.
More than 1 architecture requiring this function (ARM and ARM64 are the
two I can think of needing this immediately) sort of calls for making
the code architecture agnostic since past 2, you need something that scales.
I believe the use-case is for ARM64 at this moment.
There is already code today under kernel/dma/contiguous.c that is only
activated on a CONFIG_OF=y && CONFIG_OF_RESERVED_MEM=y system, this is
no different.
--
Florian
On Thu, Jan 07, 2021 at 10:09:14AM -0800, Florian Fainelli wrote:
quoted
On 1/7/21 9:57 AM, Konrad Rzeszutek Wilk wrote:
quoted
On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote:
quoted
Hi Greg and Konrad,
This change is intended to be non-arch specific. Any arch that lacks DMA access
control and has devices not behind an IOMMU can make use of it. Could you share
why you think this should be arch specific?
The idea behind non-arch specific code is it to be generic. The devicetree
is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should
be in arch specific code.
In premise the same code could be used with an ACPI enabled system with
an appropriate service to identify the restricted DMA regions and unlock
them.
Which this patchset is not.
ACPI is not included, but the comment about Device Tree being specific
to PowerPC, SPARC and ARM is x86 is not quite correct. There is an
architecture specific part to obtaining where the Device Tree lives in
memory, but the implementation itself is architecture agnostic (with
some early SPARC/OpenFirmware shenanigans), and x86 does, or rather did
support Device Tree to a very small extent with the CE4100 platform.
Would you prefer that an swiotlb_of.c file be created instead or
something along those lines to better encapsulate where the OF specific
code lives?
quoted
More than 1 architecture requiring this function (ARM and ARM64 are the
two I can think of needing this immediately) sort of calls for making
the code architecture agnostic since past 2, you need something that scales.
I believe the use-case is for ARM64 at this moment.
For the platforms that Claire uses, certainly for the ones we use, ARM
and ARM64 are in scope.
--
Florian
From: Jon Masters <hidden> Date: 2021-01-25 05:28:13
On 1/7/21 1:09 PM, Florian Fainelli wrote:
On 1/7/21 9:57 AM, Konrad Rzeszutek Wilk wrote:
quoted
On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote:
quoted
Hi Greg and Konrad,
This change is intended to be non-arch specific. Any arch that lacks DMA access
control and has devices not behind an IOMMU can make use of it. Could you share
why you think this should be arch specific?
The idea behind non-arch specific code is it to be generic. The devicetree
is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should
be in arch specific code.
In premise the same code could be used with an ACPI enabled system with
an appropriate service to identify the restricted DMA regions and unlock
them.
More than 1 architecture requiring this function (ARM and ARM64 are the
two I can think of needing this immediately) sort of calls for making
the code architecture agnostic since past 2, you need something that scales.
There is already code today under kernel/dma/contiguous.c that is only
activated on a CONFIG_OF=y && CONFIG_OF_RESERVED_MEM=y system, this is
no different.
<unrelated to these patches, which are useful for the case cited>
Just a note for history/archives that this approach would not be
appropriate on general purpose Arm systems, such as SystemReady-ES
edge/non-server platforms seeking to run general purpose distros. I want
to have that in the record before someone at Arm (or NVidia, or a bunch
of others that come to mind who have memory firewalls) gets an idea.
If you're working at an Arm vendor and come looking at this later
thinking "wow, what a great idea!", please fix your hardware to have a
real IOMMU/SMMU and real PCIe. You'll be pointed at this reply.
Jon.
--
Computer Architect
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-01-13 01:54:02
On 2021-01-07 17:57, Konrad Rzeszutek Wilk wrote:
On Fri, Jan 08, 2021 at 01:39:18AM +0800, Claire Chang wrote:
quoted
Hi Greg and Konrad,
This change is intended to be non-arch specific. Any arch that lacks DMA access
control and has devices not behind an IOMMU can make use of it. Could you share
why you think this should be arch specific?
The idea behind non-arch specific code is it to be generic. The devicetree
is specific to PowerPC, Sparc, and ARM, and not to x86 - hence it should
be in arch specific code.
Sorry, but that's an absurd argument. By the same token you'd equally
have to claim that bits of, say, the Broadcom WiFi driver (not to
mention dozens of others) should be split out into arch code, since not
all platforms use the devicetree parts, nor the ACPI parts, nor the PCI
parts...
There is nothing architecture-specific about using devicetree as a
system description - AFAIK there *are* a handful of x86 platforms that
use it, besides even more architectures than you've listed above. It has
long been the policy that devicetree-related code for a particular
subsystem should just live within that subsystem. Sometimes if there's
enough of it it gets collected together into its own file - e.g.
drivers/pci/of.c - otherwise it tends to just get #ifdef'ed - e.g.
of_spi_parse_dt(), or the other DMA reserved-memory consumers that
already exist as Florian points out.
Besides, there are far more platforms that enable CONFIG_OF than enable
CONFIG_SWIOTLB, so by that metric the whole of the SWIOTLB code itself
is even less "generic" than any DT parsing :P
Robin.
Maybe write it as this is "firmware adjustable" such that when/if ACPI
needs something like this, the description does not need updating.
[snip]
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
+
+ if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
MEMREMAP_WB sounds appropriate as a default.
Documentation/devicetree/bindings/reserved-memory/ramoops.txt does
define an "unbuffered" property which in premise could be applied to the
generic reserved memory binding as well and that we may have to be
honoring here, if we were to make it more generic. Oh well, this does
not need to be addressed right now I guess.
--
Florian
*
* @start: The start address of the swiotlb memory pool. Used to do a quick
* range check to see if the memory was in fact allocated by this
- * API.
+ * API. For restricted DMA pool, this is device tree adjustable.
Maybe write it as this is "firmware adjustable" such that when/if ACPI
needs something like this, the description does not need updating.
[snip]
quoted
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
+
+ if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
MEMREMAP_WB sounds appropriate as a default.
As per the binding 'no-map' has to be disabled here. So AFAIU, this memory will
be part of the linear mapping. Is this really needed then?
Documentation/devicetree/bindings/reserved-memory/ramoops.txt does
define an "unbuffered" property which in premise could be applied to the
generic reserved memory binding as well and that we may have to be
honoring here, if we were to make it more generic. Oh well, this does
not need to be addressed right now I guess.
Maybe write it as this is "firmware adjustable" such that when/if ACPI
needs something like this, the description does not need updating.
TBH I really don't think this needs calling out at all. Even in the
regular case, the details of exactly how and where the pool is allocated
are beyond the scope of this code - architectures already have several
ways to control that and make their own decisions.
quoted
[snip]
quoted
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
+
+ if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
MEMREMAP_WB sounds appropriate as a default.
As per the binding 'no-map' has to be disabled here. So AFAIU, this memory will
be part of the linear mapping. Is this really needed then?
More than that, I'd assume that we *have* to use the linear/direct map
address rather than anything that has any possibility of being a vmalloc
remap, otherwise we can no longer safely rely on
phys_to_dma/dma_to_phys, no?
That said, given that we're not actually using the returned address, I'm
not entirely sure what the point of this call is anyway. If we can
assume it's always going to return the linear map address via
try_ram_remap() then we can equally just go ahead and use the linear map
address straight away. I don't really see how we could ever hit the
"is_ram == REGION_MIXED" case in memremap() that would return NULL, if
we passed the memblock check earlier in __reserved_mem_alloc_size() such
that this rmem node ever got to be initialised at all.
Robin.
quoted
Documentation/devicetree/bindings/reserved-memory/ramoops.txt does
define an "unbuffered" property which in premise could be applied to the
generic reserved memory binding as well and that we may have to be
honoring here, if we were to make it more generic. Oh well, this does
not need to be addressed right now I guess.
*
* @start: The start address of the swiotlb memory pool. Used
to do a quick
* range check to see if the memory was in fact allocated
by this
- * API.
+ * API. For restricted DMA pool, this is device tree
adjustable.
Maybe write it as this is "firmware adjustable" such that when/if ACPI
needs something like this, the description does not need updating.
TBH I really don't think this needs calling out at all. Even in the
regular case, the details of exactly how and where the pool is allocated
are beyond the scope of this code - architectures already have several
ways to control that and make their own decisions.
quoted
quoted
[snip]
quoted
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
+
+ if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
MEMREMAP_WB sounds appropriate as a default.
As per the binding 'no-map' has to be disabled here. So AFAIU, this
memory will
be part of the linear mapping. Is this really needed then?
More than that, I'd assume that we *have* to use the linear/direct map
address rather than anything that has any possibility of being a vmalloc
remap, otherwise we can no longer safely rely on
phys_to_dma/dma_to_phys, no?
I believe you are right, which means that if we want to make use of the
restricted DMA pool on a 32-bit architecture (and we do, at least, I do)
we should probably add some error checking/warning to ensure the
restricted DMA pool falls within the linear map.
--
Florian
to do a quick
* range check to see if the memory was in fact allocated
by this
- * API.
+ * API. For restricted DMA pool, this is device tree
adjustable.
Maybe write it as this is "firmware adjustable" such that when/if ACPI
needs something like this, the description does not need updating.
TBH I really don't think this needs calling out at all. Even in the
regular case, the details of exactly how and where the pool is allocated
are beyond the scope of this code - architectures already have several
ways to control that and make their own decisions.
quoted
quoted
[snip]
quoted
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
+
+ if (!memremap(rmem->base, rmem->size, MEMREMAP_WB)) {
MEMREMAP_WB sounds appropriate as a default.
As per the binding 'no-map' has to be disabled here. So AFAIU, this
memory will
be part of the linear mapping. Is this really needed then?
More than that, I'd assume that we *have* to use the linear/direct map
address rather than anything that has any possibility of being a vmalloc
remap, otherwise we can no longer safely rely on
phys_to_dma/dma_to_phys, no?
I believe you are right, which means that if we want to make use of the
restricted DMA pool on a 32-bit architecture (and we do, at least, I do)
we should probably add some error checking/warning to ensure the
restricted DMA pool falls within the linear map.
Oh, good point - I'm so used to 64-bit that I instinctively just blanked
out the !PageHighMem() condition in try_ram_remap(). So maybe the
original intent here *was* to effectively just implement that check, but
if so it could still do with being a lot more explicit.
Cheers,
Robin.
Please add a new config option for this code instead of always building
it when swiotlb is enabled.
quoted
+static int swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
+ size_t size)
Can you split the refactoring in swiotlb.c into one or more prep
patches?
quoted
+static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
+ struct device *dev)
+{
+ struct io_tlb_mem *mem = rmem->priv;
+ int ret;
+
+ if (dev->dma_io_tlb_mem)
+ return -EBUSY;
+
+ if (!mem) {
+ mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+ if (!mem)
+ return -ENOMEM;
What is the calling convention here that allows for a NULL and non-NULL
private data?
Since multiple devices can share the same pool, the private data,
io_tlb_mem struct, will be initialized by the first device attached to
it.
This is similar to rmem_dma_device_init() in kernel/dma/coherent.c.
I'll add a comment for it in next version.
Added a new struct, io_tlb_mem, as the IO TLB memory pool descriptor and
moved relevant global variables into that struct.
This will be useful later to allow for restricted DMA pool.
Signed-off-by: Claire Chang <redacted>
---
arch/powerpc/platforms/pseries/svm.c | 4 +-
drivers/xen/swiotlb-xen.c | 4 +-
include/linux/swiotlb.h | 39 +++-
kernel/dma/swiotlb.c | 292 +++++++++++++--------------
4 files changed, 178 insertions(+), 161 deletions(-)
@@ -200,57 +170,59 @@ void swiotlb_print_info(void)*/void__initswiotlb_update_mem_attributes(void){+structio_tlb_mem*mem=&io_tlb_default_mem;void*vaddr;unsignedlongbytes;if(no_iotlb_memory||late_alloc)return;-vaddr=phys_to_virt(io_tlb_start);-bytes=PAGE_ALIGN(io_tlb_nslabs<<IO_TLB_SHIFT);+vaddr=phys_to_virt(mem->start);+bytes=PAGE_ALIGN(mem->nslabs<<IO_TLB_SHIFT);set_memory_decrypted((unsignedlong)vaddr,bytes>>PAGE_SHIFT);memset(vaddr,0,bytes);}int__initswiotlb_init_with_tbl(char*tlb,unsignedlongnslabs,intverbose){+structio_tlb_mem*mem=&io_tlb_default_mem;unsignedlongi,bytes;size_talloc_size;bytes=nslabs<<IO_TLB_SHIFT;-io_tlb_nslabs=nslabs;-io_tlb_start=__pa(tlb);-io_tlb_end=io_tlb_start+bytes;+mem->nslabs=nslabs;+mem->start=__pa(tlb);+mem->end=mem->start+bytes;/**Allocateandinitializethefreelistarray.Thisarrayisused*tofindcontiguousfreememoryregionsofsizeuptoIO_TLB_SEGSIZE-*betweenio_tlb_startandio_tlb_end.+*betweenmem->startandmem->end.*/-alloc_size=PAGE_ALIGN(io_tlb_nslabs*sizeof(int));-io_tlb_list=memblock_alloc(alloc_size,PAGE_SIZE);-if(!io_tlb_list)+alloc_size=PAGE_ALIGN(mem->nslabs*sizeof(int));+mem->list=memblock_alloc(alloc_size,PAGE_SIZE);+if(!mem->list)panic("%s: Failed to allocate %zu bytes align=0x%lx\n",__func__,alloc_size,PAGE_SIZE);-alloc_size=PAGE_ALIGN(io_tlb_nslabs*sizeof(phys_addr_t));-io_tlb_orig_addr=memblock_alloc(alloc_size,PAGE_SIZE);-if(!io_tlb_orig_addr)+alloc_size=PAGE_ALIGN(mem->nslabs*sizeof(phys_addr_t));+mem->orig_addr=memblock_alloc(alloc_size,PAGE_SIZE);+if(!mem->orig_addr)panic("%s: Failed to allocate %zu bytes align=0x%lx\n",__func__,alloc_size,PAGE_SIZE);-for(i=0;i<io_tlb_nslabs;i++){-io_tlb_list[i]=IO_TLB_SEGSIZE-OFFSET(i,IO_TLB_SEGSIZE);-io_tlb_orig_addr[i]=INVALID_PHYS_ADDR;+for(i=0;i<mem->nslabs;i++){+mem->list[i]=IO_TLB_SEGSIZE-OFFSET(i,IO_TLB_SEGSIZE);+mem->orig_addr[i]=INVALID_PHYS_ADDR;}-io_tlb_index=0;+mem->index=0;no_iotlb_memory=false;if(verbose)swiotlb_print_info();-swiotlb_set_max_segment(io_tlb_nslabs<<IO_TLB_SHIFT);+swiotlb_set_max_segment(mem->nslabs<<IO_TLB_SHIFT);return0;}
@@ -261,26 +233,27 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)void__initswiotlb_init(intverbose){+structio_tlb_mem*mem=&io_tlb_default_mem;size_tdefault_size=IO_TLB_DEFAULT_SIZE;unsignedchar*vstart;unsignedlongbytes;-if(!io_tlb_nslabs){-io_tlb_nslabs=(default_size>>IO_TLB_SHIFT);-io_tlb_nslabs=ALIGN(io_tlb_nslabs,IO_TLB_SEGSIZE);+if(!mem->nslabs){+mem->nslabs=(default_size>>IO_TLB_SHIFT);+mem->nslabs=ALIGN(mem->nslabs,IO_TLB_SEGSIZE);}-bytes=io_tlb_nslabs<<IO_TLB_SHIFT;+bytes=mem->nslabs<<IO_TLB_SHIFT;/* Get IO TLB memory from the low pages */vstart=memblock_alloc_low(PAGE_ALIGN(bytes),PAGE_SIZE);-if(vstart&&!swiotlb_init_with_tbl(vstart,io_tlb_nslabs,verbose))+if(vstart&&!swiotlb_init_with_tbl(vstart,mem->nslabs,verbose))return;-if(io_tlb_start){-memblock_free_early(io_tlb_start,-PAGE_ALIGN(io_tlb_nslabs<<IO_TLB_SHIFT));-io_tlb_start=0;+if(mem->start){+memblock_free_early(mem->start,+PAGE_ALIGN(mem->nslabs<<IO_TLB_SHIFT));+mem->start=0;}pr_warn("Cannot allocate buffer");no_iotlb_memory=true;
From: Christoph Hellwig <hch@lst.de> Date: 2021-01-13 11:51:33
On Wed, Jan 06, 2021 at 11:41:19AM +0800, Claire Chang wrote:
Added a new struct, io_tlb_mem, as the IO TLB memory pool descriptor and
moved relevant global variables into that struct.
This will be useful later to allow for restricted DMA pool.
I like where this is going, but a few comments.
Mostly I'd love to be able to entirely hide io_tlb_default_mem
and struct io_tlb_mem inside of swiotlb.c.
@@ -192,8 +192,8 @@ int __ref xen_swiotlb_init(int verbose, bool early)/**IOTLBmemoryalreadyallocated.Justuseit.*/-if(io_tlb_start!=0){-xen_io_tlb_start=phys_to_virt(io_tlb_start);+if(io_tlb_default_mem.start!=0){+xen_io_tlb_start=phys_to_virt(io_tlb_default_mem.start);gotoend;
xen_io_tlb_start is interesting. It is used only in two functions:
1) is_xen_swiotlb_buffer, where I think we should be able to just use
is_swiotlb_buffer instead of open coding it with the extra
phys_to_virt/virt_to_phys cycle.
2) xen_swiotlb_init, where except for the assignment it only is used
locally for the case not touched above and could this be replaced
with a local variable.
Konrad, does this make sense to you?
@@ -477,15 +476,6 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr,if(no_iotlb_memory&&!hwdev->dma_io_tlb_mem)panic("Can not allocate SWIOTLB buffer earlier and can't now provide you with the DMA bounce buffer");-if(mem_encrypt_active())-pr_warn_once("Memory encryption is active and system is using DMA bounce buffers\n");--if(mapping_size>alloc_size){-dev_warn_once(hwdev,"Invalid sizes (mapping: %zd bytes, alloc: %zd bytes)",-mapping_size,alloc_size);-return(phys_addr_t)DMA_MAPPING_ERROR;-}-mask=dma_get_seg_boundary(hwdev);tbl_dma_addr&=mask;
@@ -570,45 +559,21 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr,if(!(attrs&DMA_ATTR_NO_WARN)&&printk_ratelimit())dev_warn(hwdev,"swiotlb buffer is full (sz: %zd bytes), total %lu (slots), used %lu (slots)\n",alloc_size,mem->nslabs,tmp_io_tlb_used);-return(phys_addr_t)DMA_MAPPING_ERROR;+return-ENOMEM;+found:mem->used+=nslots;spin_unlock_irqrestore(&mem->lock,flags);-/*-*SaveawaythemappingfromtheoriginaladdresstotheDMAaddress.-*Thisisneededwhenwesyncthememory.Thenwesyncthebufferif-*needed.-*/-for(i=0;i<nslots;i++)-mem->orig_addr[index+i]=orig_addr+(i<<IO_TLB_SHIFT);-if(!(attrs&DMA_ATTR_SKIP_CPU_SYNC)&&-(dir==DMA_TO_DEVICE||dir==DMA_BIDIRECTIONAL))-swiotlb_bounce(orig_addr,tlb_addr,mapping_size,DMA_TO_DEVICE);--returntlb_addr;+returnindex;}-/*-*tlb_addristhephysicaladdressofthebouncebuffertounmap.-*/-voidswiotlb_tbl_unmap_single(structdevice*hwdev,phys_addr_ttlb_addr,-size_tmapping_size,size_talloc_size,-enumdma_data_directiondir,unsignedlongattrs)+staticvoidswiotlb_tbl_release_region(structdevice*hwdev,intindex,+size_tsize){structio_tlb_mem*mem=get_io_tlb_mem(hwdev);unsignedlongflags;-inti,count,nslots=ALIGN(alloc_size,1<<IO_TLB_SHIFT)>>IO_TLB_SHIFT;-intindex=(tlb_addr-mem->start)>>IO_TLB_SHIFT;-phys_addr_torig_addr=mem->orig_addr[index];--/*-*First,syncthememorybeforeunmappingtheentry-*/-if(orig_addr!=INVALID_PHYS_ADDR&&-!(attrs&DMA_ATTR_SKIP_CPU_SYNC)&&-((dir==DMA_FROM_DEVICE)||(dir==DMA_BIDIRECTIONAL)))-swiotlb_bounce(orig_addr,tlb_addr,mapping_size,DMA_FROM_DEVICE);+inti,count,nslots=ALIGN(size,1<<IO_TLB_SHIFT)>>IO_TLB_SHIFT;/**Returnthebuffertothefreelistbysettingthecorresponding
@@ -640,6 +605,69 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,spin_unlock_irqrestore(&mem->lock,flags);}+phys_addr_tswiotlb_tbl_map_single(structdevice*hwdev,phys_addr_torig_addr,+size_tmapping_size,size_talloc_size,+enumdma_data_directiondir,unsignedlongattrs)+{+structio_tlb_mem*mem=get_io_tlb_mem(hwdev);+dma_addr_ttbl_dma_addr=phys_to_dma_unencrypted(hwdev,mem->start);+phys_addr_ttlb_addr;+unsignedintnslots,index;+inti;++if(mem_encrypt_active())+pr_warn_once("Memory encryption is active and system is using DMA bounce buffers\n");++if(mapping_size>alloc_size){+dev_warn_once(hwdev,"Invalid sizes (mapping: %zd bytes, alloc: %zd bytes)",+mapping_size,alloc_size);+return(phys_addr_t)DMA_MAPPING_ERROR;+}++index=swiotlb_tbl_find_free_region(hwdev,tbl_dma_addr,alloc_size,+attrs);+if(index<0)+return(phys_addr_t)DMA_MAPPING_ERROR;++tlb_addr=mem->start+(index<<IO_TLB_SHIFT);++/*+*SaveawaythemappingfromtheoriginaladdresstotheDMAaddress.+*Thisisneededwhenwesyncthememory.Thenwesyncthebufferif+*needed.+*/+nslots=ALIGN(alloc_size,1<<IO_TLB_SHIFT)>>IO_TLB_SHIFT;+for(i=0;i<nslots;i++)+mem->orig_addr[index+i]=orig_addr+(i<<IO_TLB_SHIFT);+if(!(attrs&DMA_ATTR_SKIP_CPU_SYNC)&&+(dir==DMA_TO_DEVICE||dir==DMA_BIDIRECTIONAL))+swiotlb_bounce(orig_addr,tlb_addr,mapping_size,DMA_TO_DEVICE);++returntlb_addr;+}++/*+*tlb_addristhephysicaladdressofthebouncebuffertounmap.+*/+voidswiotlb_tbl_unmap_single(structdevice*hwdev,phys_addr_ttlb_addr,+size_tmapping_size,size_talloc_size,+enumdma_data_directiondir,unsignedlongattrs)+{+structio_tlb_mem*mem=get_io_tlb_mem(hwdev);+intindex=(tlb_addr-mem->start)>>IO_TLB_SHIFT;+phys_addr_torig_addr=mem->orig_addr[index];++/*+*First,syncthememorybeforeunmappingtheentry+*/+if(orig_addr!=INVALID_PHYS_ADDR&&+!(attrs&DMA_ATTR_SKIP_CPU_SYNC)&&+((dir==DMA_FROM_DEVICE)||(dir==DMA_BIDIRECTIONAL)))+swiotlb_bounce(orig_addr,tlb_addr,mapping_size,DMA_FROM_DEVICE);++swiotlb_tbl_release_region(hwdev,index,alloc_size);+}+voidswiotlb_tbl_sync_single(structdevice*hwdev,phys_addr_ttlb_addr,size_tsize,enumdma_data_directiondir,enumdma_sync_targettarget)
@@ -706,6 +734,59 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t paddr, size_t size,returndma_addr;}+void*swiotlb_alloc(structdevice*dev,size_tsize,dma_addr_t*dma_handle,+unsignedlongattrs)+{+structio_tlb_mem*mem=dev->dma_io_tlb_mem;+intindex;+void*vaddr;+phys_addr_ttlb_addr;++size=PAGE_ALIGN(size);+index=swiotlb_tbl_find_free_region(dev,mem->start,size,attrs);+if(index<0)+returnNULL;++tlb_addr=mem->start+(index<<IO_TLB_SHIFT);+*dma_handle=phys_to_dma_unencrypted(dev,tlb_addr);++if(!dev_is_dma_coherent(dev)){+unsignedlongpfn=PFN_DOWN(tlb_addr);++/* remove any dirty cache lines on the kernel alias */+arch_dma_prep_coherent(pfn_to_page(pfn),size);++/* create a coherent mapping */+vaddr=dma_common_contiguous_remap(+pfn_to_page(pfn),size,+dma_pgprot(dev,PAGE_KERNEL,attrs),+__builtin_return_address(0));+if(!vaddr){+swiotlb_tbl_release_region(dev,index,size);+returnNULL;+}+}else{+vaddr=phys_to_virt(tlb_addr);+}++memset(vaddr,0,size);++returnvaddr;+}++voidswiotlb_free(structdevice*dev,size_tsize,void*vaddr,+dma_addr_tdma_addr,unsignedlongattrs)+{+structio_tlb_mem*mem=dev->dma_io_tlb_mem;+unsignedintindex;++if(!dev_is_dma_coherent(dev))+vunmap(vaddr);++index=(dma_addr-mem->start)>>IO_TLB_SHIFT;+swiotlb_tbl_release_region(dev,index,PAGE_ALIGN(size));+}+size_tswiotlb_max_mapping_size(structdevice*dev){return((size_t)1<<IO_TLB_SHIFT)*IO_TLB_SEGSIZE;
Add the functions, swiotlb_alloc and swiotlb_free to support the
memory allocation from restricted DMA pool.
Signed-off-by: Claire Chang <redacted>
---
I'd rather have the names convey there are for the per-device bounce
buffer in some form.
+ struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
While we're at it I wonder if the io_tlb is something we could change
while we're at it. Maybe replace io_tlb_mem with struct swiotlb
and rename the field in struct device to dev_swiotlb?
+ int index;
+ void *vaddr;
+ phys_addr_t tlb_addr;
+
+ size = PAGE_ALIGN(size);
+ index = swiotlb_tbl_find_free_region(dev, mem->start, size, attrs);
+ if (index < 0)
+ return NULL;
+
+ tlb_addr = mem->start + (index << IO_TLB_SHIFT);
+ *dma_handle = phys_to_dma_unencrypted(dev, tlb_addr);
+
+ if (!dev_is_dma_coherent(dev)) {
+ unsigned long pfn = PFN_DOWN(tlb_addr);
+
+ /* remove any dirty cache lines on the kernel alias */
+ arch_dma_prep_coherent(pfn_to_page(pfn), size);
Can we hook in somewhat lower level in the dma-direct code so that all
the remapping in dma-direct can be reused instead of duplicated? That
also becomes important if we want to use non-remapping uncached support,
e.g. on mips or x86, or the direct changing of the attributes that Will
planned to look into for arm64.
I'd rather have the names convey there are for the per-device bounce
buffer in some form.
quoted
+ struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
While we're at it I wonder if the io_tlb is something we could change
while we're at it. Maybe replace io_tlb_mem with struct swiotlb
and rename the field in struct device to dev_swiotlb?
quoted
+ int index;
+ void *vaddr;
+ phys_addr_t tlb_addr;
+
+ size = PAGE_ALIGN(size);
+ index = swiotlb_tbl_find_free_region(dev, mem->start, size, attrs);
+ if (index < 0)
+ return NULL;
+
+ tlb_addr = mem->start + (index << IO_TLB_SHIFT);
+ *dma_handle = phys_to_dma_unencrypted(dev, tlb_addr);
+
+ if (!dev_is_dma_coherent(dev)) {
+ unsigned long pfn = PFN_DOWN(tlb_addr);
+
+ /* remove any dirty cache lines on the kernel alias */
+ arch_dma_prep_coherent(pfn_to_page(pfn), size);
Can we hook in somewhat lower level in the dma-direct code so that all
the remapping in dma-direct can be reused instead of duplicated? That
also becomes important if we want to use non-remapping uncached support,
e.g. on mips or x86, or the direct changing of the attributes that Will
planned to look into for arm64.
Indeed, AFAICS this ought to boil down to a direct equivalent of
__dma_direct_alloc_pages() - other than the address there should be no
conceptual difference between pages from the restricted pool and those
from the regular page allocator, so this probably deserves to be plumbed
in as an alternative to that.
Robin.
From: Christoph Hellwig <hch@lst.de> Date: 2021-01-13 18:33:10
On Wed, Jan 13, 2021 at 06:27:08PM +0000, Robin Murphy wrote:
quoted
Can we hook in somewhat lower level in the dma-direct code so that all
the remapping in dma-direct can be reused instead of duplicated? That
also becomes important if we want to use non-remapping uncached support,
e.g. on mips or x86, or the direct changing of the attributes that Will
planned to look into for arm64.
Indeed, AFAICS this ought to boil down to a direct equivalent of
__dma_direct_alloc_pages() - other than the address there should be no
conceptual difference between pages from the restricted pool and those from
the regular page allocator, so this probably deserves to be plumbed in as
an alternative to that.
Yes, that's what I mean. You managed to word it much better, though.
Regardless of swiotlb setting, the restricted DMA pool is preferred if
available.
The restricted DMA pools provide a basic level of protection against
the DMA overwriting buffer contents at unexpected times. However, to
protect against general data leakage and system memory corruption, the
system needs to provide a way to restrict the DMA to a predefined memory
region.
Signed-off-by: Claire Chang <redacted>
---
drivers/iommu/dma-iommu.c | 12 ++++++------
include/linux/swiotlb.h | 17 +++++++++++------
kernel/dma/direct.c | 8 ++++----
kernel/dma/direct.h | 10 ++++++----
kernel/dma/swiotlb.c | 13 ++++++-------
5 files changed, 33 insertions(+), 27 deletions(-)
@@ -495,7 +495,7 @@ int dma_direct_supported(struct device *dev, u64 mask)size_tdma_direct_max_mapping_size(structdevice*dev){/* If SWIOTLB is active, use its maximum mapping size */-if(is_swiotlb_active()&&+if(is_swiotlb_active(dev)&&(dma_addressing_limited(dev)||swiotlb_force==SWIOTLB_FORCE))returnswiotlb_max_mapping_size(dev);returnSIZE_MAX;
@@ -222,7 +222,6 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)mem->orig_addr[i]=INVALID_PHYS_ADDR;}mem->index=0;-no_iotlb_memory=false;if(verbose)swiotlb_print_info();
@@ -475,7 +474,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t orig_addr,unsignedlongmax_slots;unsignedlongtmp_io_tlb_used;-if(no_iotlb_memory)+if(no_iotlb_memory&&!hwdev->dma_io_tlb_mem)panic("Can not allocate SWIOTLB buffer earlier and can't now provide you with the DMA bounce buffer");if(mem_encrypt_active())
Regardless of swiotlb setting, the restricted DMA pool is preferred if
available.
The restricted DMA pools provide a basic level of protection against
the DMA overwriting buffer contents at unexpected times. However, to
protect against general data leakage and system memory corruption, the
system needs to provide a way to restrict the DMA to a predefined memory
region.
Signed-off-by: Claire Chang <redacted>
You could probably split this patch into two:
- one that introduces the get_io_tlb_mem() getter, updates all callers
of is_swiotlb_buffer() to gain a 'struct device' argument
- another one that does add support for a non-default swiotlb pool and
adds dev->dma_io_tlb_mem
Other than that, LGTM!
--
Florian
@@ -222,7 +222,6 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)mem->orig_addr[i]=INVALID_PHYS_ADDR;}mem->index=0;-no_iotlb_memory=false;
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
Signed-off-by: Claire Chang <redacted>
---
.../reserved-memory/reserved-memory.txt | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
@@ -51,6 +51,20 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to restrict+ the DMA to a predefined memory region. - vendor specific string in the form <vendor>,[<device>-]<usage> no-map (optional) - empty property - Indicates the operating system must not create a virtual mapping
From: Konrad Rzeszutek Wilk <hidden> Date: 2021-01-06 18:59:49
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
quoted hunk
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
Signed-off-by: Claire Chang <redacted>
---
.../reserved-memory/reserved-memory.txt | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
@@ -51,6 +51,20 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to restrict+ the DMA to a predefined memory region.
Heya!
I think I am missing something obvious here so please bear with my
questions:
- This code adds the means of having the SWIOTLB pool tied to a specific
memory correct?
- Nothing stops the physical device from bypassing the SWIOTLB buffer.
That is if an errant device screwed up the length or DMA address, the
SWIOTLB would gladly do what the device told it do?
- This has to be combined with SWIOTLB-force-ish to always use the
bounce buffer, otherwise you could still do DMA without using
SWIOTLB (by not hitting the criteria for needing to use SWIOTLB)?
On Thu, Jan 7, 2021 at 2:58 AM Konrad Rzeszutek Wilk
[off-list ref] wrote:
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
quoted
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
Signed-off-by: Claire Chang <redacted>
---
.../reserved-memory/reserved-memory.txt | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
@@ -51,6 +51,20 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to restrict+ the DMA to a predefined memory region.
Heya!
I think I am missing something obvious here so please bear with my
questions:
- This code adds the means of having the SWIOTLB pool tied to a specific
memory correct?
It doesn't affect the existing SWIOTLB. It just utilizes the existing SWIOTLB
code to create another DMA pool tied to a specific memory region for a given set
of devices. It bounces the streaming DMA (map/unmap) in and out of that region
and does the memory allocation (dma_direct_alloc) from the same region.
- Nothing stops the physical device from bypassing the SWIOTLB buffer.
That is if an errant device screwed up the length or DMA address, the
SWIOTLB would gladly do what the device told it do?
So the system needs to provide a way to lock down the memory access, e.g. MPU.
- This has to be combined with SWIOTLB-force-ish to always use the
bounce buffer, otherwise you could still do DMA without using
SWIOTLB (by not hitting the criteria for needing to use SWIOTLB)?
Since restricted DMA is for the devices that are not behind an IOMMU, I change
the criteria
`if (unlikely(swiotlb_force == SWIOTLB_FORCE))`
to
`if (unlikely(swiotlb_force == SWIOTLB_FORCE) || dev->dma_io_tlb_mem)`
in dma_direct_map_page().
Also, even if SWIOTLB=force, the restricted DMA pool is preferred if available
(get_io_tlb_mem in https://lore.kernel.org/patchwork/patch/1360995/).
Thanks!
From: Konrad Rzeszutek Wilk <hidden> Date: 2021-01-07 18:06:01
On Fri, Jan 08, 2021 at 01:39:43AM +0800, Claire Chang wrote:
On Thu, Jan 7, 2021 at 2:58 AM Konrad Rzeszutek Wilk
[off-list ref] wrote:
quoted
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
quoted
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
Signed-off-by: Claire Chang <redacted>
---
.../reserved-memory/reserved-memory.txt | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
@@ -51,6 +51,20 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to restrict+ the DMA to a predefined memory region.
Heya!
I think I am missing something obvious here so please bear with my
questions:
- This code adds the means of having the SWIOTLB pool tied to a specific
memory correct?
It doesn't affect the existing SWIOTLB. It just utilizes the existing SWIOTLB
code to create another DMA pool tied to a specific memory region for a given set
of devices. It bounces the streaming DMA (map/unmap) in and out of that region
and does the memory allocation (dma_direct_alloc) from the same region.
Right, so why can't it follow the same mechanism that Xen SWIOTLB does - which
had exactly the same problem (needed special handling on the pool) - and do
a similar code?
quoted
- Nothing stops the physical device from bypassing the SWIOTLB buffer.
That is if an errant device screwed up the length or DMA address, the
SWIOTLB would gladly do what the device told it do?
So the system needs to provide a way to lock down the memory access, e.g. MPU.
OK! Would it be prudent to have this in the description above perhaps?
quoted
- This has to be combined with SWIOTLB-force-ish to always use the
bounce buffer, otherwise you could still do DMA without using
SWIOTLB (by not hitting the criteria for needing to use SWIOTLB)?
Since restricted DMA is for the devices that are not behind an IOMMU, I change
the criteria
`if (unlikely(swiotlb_force == SWIOTLB_FORCE))`
to
`if (unlikely(swiotlb_force == SWIOTLB_FORCE) || dev->dma_io_tlb_mem)`
in dma_direct_map_page().
Also, even if SWIOTLB=force, the restricted DMA pool is preferred if available
(get_io_tlb_mem in https://lore.kernel.org/patchwork/patch/1360995/).
Thanks!
- Nothing stops the physical device from bypassing the SWIOTLB buffer.
That is if an errant device screwed up the length or DMA address, the
SWIOTLB would gladly do what the device told it do?
So the system needs to provide a way to lock down the memory access, e.g. MPU.
OK! Would it be prudent to have this in the description above perhaps?
Yes this is something that must be documented as a requirement for the
restricted DMA pool users, otherwise attempting to do restricted DMA
pool is no different than say, using a device private CMA region.
Without the enforcement, this is just a best effort.
--
Florian
On Fri, Jan 8, 2021 at 2:15 AM Florian Fainelli [off-list ref] wrote:
On 1/7/21 10:00 AM, Konrad Rzeszutek Wilk wrote:
quoted
quoted
quoted
- Nothing stops the physical device from bypassing the SWIOTLB buffer.
That is if an errant device screwed up the length or DMA address, the
SWIOTLB would gladly do what the device told it do?
So the system needs to provide a way to lock down the memory access, e.g. MPU.
OK! Would it be prudent to have this in the description above perhaps?
Yes this is something that must be documented as a requirement for the
restricted DMA pool users, otherwise attempting to do restricted DMA
pool is no different than say, using a device private CMA region.
Without the enforcement, this is just a best effort.
From: Rob Herring <robh@kernel.org> Date: 2021-01-20 16:55:51
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
If this goes into DT, I think we should be able to use dma-ranges for
this purpose instead. Normally, 'dma-ranges' is for physical bus
restrictions, but there's no reason it can't be used for policy or to
express restrictions the firmware has enabled.
@@ -51,6 +51,20 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to restrict+ the DMA to a predefined memory region. - vendor specific string in the form <vendor>,[<device>-]<usage> no-map (optional) - empty property - Indicates the operating system must not create a virtual mapping
PCI hosts often have inbound window configurations that limit the
address range and translate PCI to bus addresses. Those windows happen
to be configured by dma-ranges. In any case, wouldn't you want to put
the configuration in the PCI host node? Is there a usecase of
restricting one PCIe device and not another?
Rob
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-01-20 18:42:43
On 2021-01-20 16:53, Rob Herring wrote:
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
quoted
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
If this goes into DT, I think we should be able to use dma-ranges for
this purpose instead. Normally, 'dma-ranges' is for physical bus
restrictions, but there's no reason it can't be used for policy or to
express restrictions the firmware has enabled.
There would still need to be some way to tell SWIOTLB to pick up the
corresponding chunk of memory and to prevent the kernel from using it
for anything else, though.
@@ -51,6 +51,20 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to restrict+ the DMA to a predefined memory region. - vendor specific string in the form <vendor>,[<device>-]<usage> no-map (optional) - empty property - Indicates the operating system must not create a virtual mapping
PCI hosts often have inbound window configurations that limit the
address range and translate PCI to bus addresses. Those windows happen
to be configured by dma-ranges. In any case, wouldn't you want to put
the configuration in the PCI host node? Is there a usecase of
restricting one PCIe device and not another?
The general design seems to accommodate devices having their own pools
such that they can't even snoop on each others' transient DMA data. If
the interconnect had a way of wiring up, say, PCI RIDs to AMBA NSAIDs,
then in principle you could certainly apply that to PCI endpoints too
(presumably you'd also disallow them from peer-to-peer transactions at
the PCI level too).
Robin.
From: Rob Herring <robh@kernel.org> Date: 2021-01-21 03:03:05
On Wed, Jan 20, 2021 at 11:30 AM Robin Murphy [off-list ref] wrote:
On 2021-01-20 16:53, Rob Herring wrote:
quoted
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
quoted
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
If this goes into DT, I think we should be able to use dma-ranges for
this purpose instead. Normally, 'dma-ranges' is for physical bus
restrictions, but there's no reason it can't be used for policy or to
express restrictions the firmware has enabled.
There would still need to be some way to tell SWIOTLB to pick up the
corresponding chunk of memory and to prevent the kernel from using it
for anything else, though.
Don't we already have that problem if dma-ranges had a very small
range? We just get lucky because the restriction is generally much
more RAM than needed.
In any case, wouldn't finding all the dma-ranges do this? We're
already walking the tree to find the max DMA address now.
@@ -51,6 +51,20 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to restrict+ the DMA to a predefined memory region. - vendor specific string in the form <vendor>,[<device>-]<usage> no-map (optional) - empty property - Indicates the operating system must not create a virtual mapping
PCI hosts often have inbound window configurations that limit the
address range and translate PCI to bus addresses. Those windows happen
to be configured by dma-ranges. In any case, wouldn't you want to put
the configuration in the PCI host node? Is there a usecase of
restricting one PCIe device and not another?
The general design seems to accommodate devices having their own pools
such that they can't even snoop on each others' transient DMA data. If
the interconnect had a way of wiring up, say, PCI RIDs to AMBA NSAIDs,
then in principle you could certainly apply that to PCI endpoints too
(presumably you'd also disallow them from peer-to-peer transactions at
the PCI level too).
At least for PCI, I think we can handle this. We have the BDF in the
3rd address cell in dma-ranges. The Openfirmware spec says those are 0
in the case of ranges. It doesn't talk about dma-ranges though. But I
think we could extend it to allow for BDF. Though typically with PCIe
every device is behind its own bridge and each bridge node can have a
dma-ranges.
Rob
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-01-21 01:15:00
On 2021-01-20 21:31, Rob Herring wrote:
On Wed, Jan 20, 2021 at 11:30 AM Robin Murphy [off-list ref] wrote:
quoted
On 2021-01-20 16:53, Rob Herring wrote:
quoted
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
quoted
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
If this goes into DT, I think we should be able to use dma-ranges for
this purpose instead. Normally, 'dma-ranges' is for physical bus
restrictions, but there's no reason it can't be used for policy or to
express restrictions the firmware has enabled.
There would still need to be some way to tell SWIOTLB to pick up the
corresponding chunk of memory and to prevent the kernel from using it
for anything else, though.
Don't we already have that problem if dma-ranges had a very small
range? We just get lucky because the restriction is generally much
more RAM than needed.
Not really - if a device has a naturally tiny addressing capability that
doesn't even cover ZONE_DMA32 where the regular SWIOTLB buffer will be
allocated then it's unlikely to work well, but that's just crap system
design. Yes, memory pressure in ZONE_DMA{32} is particularly problematic
for such limited devices, but it's irrelevant to the issue at hand here.
What we have here is a device that's not allowed to see *kernel* memory
at all. It's been artificially constrained to a particular region by a
TZASC or similar, and the only data which should ever be placed in that
region is data intended for that device to see. That way if it tries to
go rogue it physically can't start slurping data intended for other
devices or not mapped for DMA at all. The bouncing is an important part
of this - I forget the title off-hand but there was an interesting paper
a few years ago which demonstrated that even with an IOMMU, streaming
DMA of in-place buffers could reveal enough adjacent data from the same
page to mount an attack on the system. Memory pressure should be
immaterial since the size of each bounce pool carveout will presumably
be tuned for the needs of the given device.
In any case, wouldn't finding all the dma-ranges do this? We're
already walking the tree to find the max DMA address now.
If all you can see are two "dma-ranges" properties, how do you propose
to tell that one means "this is the extent of what I can address, please
set my masks and dma-range-map accordingly and try to allocate things
where I can reach them" while the other means "take this output range
away from the page allocator and hook it up as my dedicated bounce pool,
because it is Serious Security Time"? Especially since getting that
choice wrong either way would be a Bad Thing.
Robin.
@@ -51,6 +51,20 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to restrict+ the DMA to a predefined memory region. - vendor specific string in the form <vendor>,[<device>-]<usage> no-map (optional) - empty property - Indicates the operating system must not create a virtual mapping
PCI hosts often have inbound window configurations that limit the
address range and translate PCI to bus addresses. Those windows happen
to be configured by dma-ranges. In any case, wouldn't you want to put
the configuration in the PCI host node? Is there a usecase of
restricting one PCIe device and not another?
The general design seems to accommodate devices having their own pools
such that they can't even snoop on each others' transient DMA data. If
the interconnect had a way of wiring up, say, PCI RIDs to AMBA NSAIDs,
then in principle you could certainly apply that to PCI endpoints too
(presumably you'd also disallow them from peer-to-peer transactions at
the PCI level too).
At least for PCI, I think we can handle this. We have the BDF in the
3rd address cell in dma-ranges. The Openfirmware spec says those are 0
in the case of ranges. It doesn't talk about dma-ranges though. But I
think we could extend it to allow for BDF. Though typically with PCIe
every device is behind its own bridge and each bridge node can have a
dma-ranges.
Rob
From: Rob Herring <robh@kernel.org> Date: 2021-01-21 15:51:44
On Wed, Jan 20, 2021 at 7:10 PM Robin Murphy [off-list ref] wrote:
On 2021-01-20 21:31, Rob Herring wrote:
quoted
On Wed, Jan 20, 2021 at 11:30 AM Robin Murphy [off-list ref] wrote:
quoted
On 2021-01-20 16:53, Rob Herring wrote:
quoted
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
quoted
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the device tree.
If this goes into DT, I think we should be able to use dma-ranges for
this purpose instead. Normally, 'dma-ranges' is for physical bus
restrictions, but there's no reason it can't be used for policy or to
express restrictions the firmware has enabled.
There would still need to be some way to tell SWIOTLB to pick up the
corresponding chunk of memory and to prevent the kernel from using it
for anything else, though.
Don't we already have that problem if dma-ranges had a very small
range? We just get lucky because the restriction is generally much
more RAM than needed.
Not really - if a device has a naturally tiny addressing capability that
doesn't even cover ZONE_DMA32 where the regular SWIOTLB buffer will be
allocated then it's unlikely to work well, but that's just crap system
design. Yes, memory pressure in ZONE_DMA{32} is particularly problematic
for such limited devices, but it's irrelevant to the issue at hand here.
Yesterday's crap system design is today's security feature. Couldn't
this feature make crap system design work better?
What we have here is a device that's not allowed to see *kernel* memory
at all. It's been artificially constrained to a particular region by a
TZASC or similar, and the only data which should ever be placed in that
May have been constrained, but that's entirely optional.
In the optional case where the setup is entirely up to the OS, I don't
think this belongs in the DT at all. Perhaps that should be solved
first.
region is data intended for that device to see. That way if it tries to
go rogue it physically can't start slurping data intended for other
devices or not mapped for DMA at all. The bouncing is an important part
of this - I forget the title off-hand but there was an interesting paper
a few years ago which demonstrated that even with an IOMMU, streaming
DMA of in-place buffers could reveal enough adjacent data from the same
page to mount an attack on the system. Memory pressure should be
immaterial since the size of each bounce pool carveout will presumably
be tuned for the needs of the given device.
quoted
In any case, wouldn't finding all the dma-ranges do this? We're
already walking the tree to find the max DMA address now.
If all you can see are two "dma-ranges" properties, how do you propose
to tell that one means "this is the extent of what I can address, please
set my masks and dma-range-map accordingly and try to allocate things
where I can reach them" while the other means "take this output range
away from the page allocator and hook it up as my dedicated bounce pool,
because it is Serious Security Time"? Especially since getting that
choice wrong either way would be a Bad Thing.
Either we have some heuristic based on the size or we add some hint.
The point is let's build on what we already have for defining DMA
accessible memory in DT rather than some parallel mechanism.
Rob
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-01-21 17:34:09
On 2021-01-21 15:48, Rob Herring wrote:
On Wed, Jan 20, 2021 at 7:10 PM Robin Murphy [off-list ref]
wrote:
quoted
On 2021-01-20 21:31, Rob Herring wrote:
quoted
On Wed, Jan 20, 2021 at 11:30 AM Robin Murphy
[off-list ref] wrote:
quoted
On 2021-01-20 16:53, Rob Herring wrote:
quoted
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang
wrote:
quoted
Introduce the new compatible string, restricted-dma-pool,
for restricted DMA. One can specify the address and length
of the restricted DMA memory region by restricted-dma-pool
in the device tree.
If this goes into DT, I think we should be able to use
dma-ranges for this purpose instead. Normally, 'dma-ranges'
is for physical bus restrictions, but there's no reason it
can't be used for policy or to express restrictions the
firmware has enabled.
There would still need to be some way to tell SWIOTLB to pick
up the corresponding chunk of memory and to prevent the kernel
from using it for anything else, though.
Don't we already have that problem if dma-ranges had a very
small range? We just get lucky because the restriction is
generally much more RAM than needed.
Not really - if a device has a naturally tiny addressing capability
that doesn't even cover ZONE_DMA32 where the regular SWIOTLB buffer
will be allocated then it's unlikely to work well, but that's just
crap system design. Yes, memory pressure in ZONE_DMA{32} is
particularly problematic for such limited devices, but it's
irrelevant to the issue at hand here.
Yesterday's crap system design is today's security feature. Couldn't
this feature make crap system design work better?
Indeed! Say you bring out your shiny new "Strawberry Flan 4" machine
with all the latest connectivity, but tragically its PCIe can only
address 25% of the RAM. So you decide to support deploying it in two
configurations: one where it runs normally for best performance, and
another "secure" one where it dedicates that quarter of RAM as a
restricted DMA pool for any PCIe devices - that way, even if that hotel
projector you plug in turns out to be a rogue Thunderbolt endpoint, it
can never snarf your private keys off your eMMC out of the page cache.
(Yes, is is the thinnest of strawmen, but it sets the scene for the
point you raised...)
...which is that in both cases the dma-ranges will still be identical.
So how is the kernel going to know whether to steal that whole area from
memblock before anything else can allocate from it, or not?
I don't disagree that even in Claire's original intended case it would
be semantically correct to describe the hardware-firewalled region with
dma-ranges. It just turns out not to be necessary, and you're already
arguing for not adding anything in DT that doesn't need to be.
quoted
What we have here is a device that's not allowed to see *kernel*
memory at all. It's been artificially constrained to a particular
region by a TZASC or similar, and the only data which should ever
be placed in that
May have been constrained, but that's entirely optional.
In the optional case where the setup is entirely up to the OS, I
don't think this belongs in the DT at all. Perhaps that should be
solved first.
Yes! Let's definitely consider that case! Say you don't have any
security or physical limitations but want to use a bounce pool for some
device anyway because reasons (perhaps copying streaming DMA data to a
better guaranteed alignment gives an overall performance win). Now the
*only* relevant thing to communicate to the kernel is to, ahem, reserve
a large chunk of memory, and use it for this special purpose. Isn't that
literally what reserved-memory bindings are for?
quoted
region is data intended for that device to see. That way if it
tries to go rogue it physically can't start slurping data intended
for other devices or not mapped for DMA at all. The bouncing is an
important part of this - I forget the title off-hand but there was
an interesting paper a few years ago which demonstrated that even
with an IOMMU, streaming DMA of in-place buffers could reveal
enough adjacent data from the same page to mount an attack on the
system. Memory pressure should be immaterial since the size of each
bounce pool carveout will presumably be tuned for the needs of the
given device.
quoted
In any case, wouldn't finding all the dma-ranges do this? We're
already walking the tree to find the max DMA address now.
If all you can see are two "dma-ranges" properties, how do you
propose to tell that one means "this is the extent of what I can
address, please set my masks and dma-range-map accordingly and try
to allocate things where I can reach them" while the other means
"take this output range away from the page allocator and hook it up
as my dedicated bounce pool, because it is Serious Security Time"?
Especially since getting that choice wrong either way would be a
Bad Thing.
Either we have some heuristic based on the size or we add some hint.
The point is let's build on what we already have for defining DMA
accessible memory in DT rather than some parallel mechanism.
The point I'm trying to bang home is that it's really not about the DMA
accessibility, it's about the purpose of the memory itself. Even when
DMA accessibility *is* relevant it's already implied by that purpose,
from the point of view of the implementation. The only difference it
might make is to the end user if they want to ascertain whether the
presence of such a pool represents protection against an untrusted
device or just some DMA optimisation tweak.
Robin.
If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.
Signed-off-by: Claire Chang <redacted>
---
drivers/of/address.c | 21 +++++++++++++++++++++
drivers/of/device.c | 4 ++++
drivers/of/of_private.h | 5 +++++
3 files changed, 30 insertions(+)
If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.
Signed-off-by: Claire Chang <redacted>
---
You could have an early check for count < 0, along with an error
message, if that is deemed useful.
+ for (i = 0; i < count; i++) {
+ node = of_parse_phandle(dev->of_node, "memory-region", i);
+ if (of_device_is_compatible(node, "restricted-dma-pool"))
And you may want to add here an of_device_is_available(node). A platform
that provides the Device Tree firmware and try to support multiple
different SoCs may try to determine if an IOMMU is present, and if it
is, it could be marking the restriced-dma-pool region with a 'status =
"disabled"' property, or any variant of that scheme.
This does not seem to be supporting more than one memory region, did not
you want something like instead:
ret = of_reserved_mem_device_init_by_idx(...);
if (ret)
return ret;
On Wed, Jan 13, 2021 at 7:48 AM Florian Fainelli [off-list ref] wrote:
On 1/5/21 7:41 PM, Claire Chang wrote:
quoted
If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.
Signed-off-by: Claire Chang <redacted>
---
You could have an early check for count < 0, along with an error
message, if that is deemed useful.
quoted
+ for (i = 0; i < count; i++) {
+ node = of_parse_phandle(dev->of_node, "memory-region", i);
+ if (of_device_is_compatible(node, "restricted-dma-pool"))
And you may want to add here an of_device_is_available(node). A platform
that provides the Device Tree firmware and try to support multiple
different SoCs may try to determine if an IOMMU is present, and if it
is, it could be marking the restriced-dma-pool region with a 'status =
"disabled"' property, or any variant of that scheme.
This function is called only when there is no IOMMU present (check in
drivers/of/device.c). I can still add of_device_is_available(node)
here if you think it's helpful.
This does not seem to be supporting more than one memory region, did not
you want something like instead:
ret = of_reserved_mem_device_init_by_idx(...);
if (ret)
return ret;
Yes. This implement only supports one restriced-dma-pool memory region
with the assumption that there is only one memory region with the
compatible string, restricted-dma-pool, in the dts. IIUC, it's similar
to shared-dma-pool.
On Wed, Jan 13, 2021 at 7:48 AM Florian Fainelli [off-list ref] wrote:
quoted
On 1/5/21 7:41 PM, Claire Chang wrote:
quoted
If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.
Signed-off-by: Claire Chang <redacted>
---
You could have an early check for count < 0, along with an error
message, if that is deemed useful.
quoted
+ for (i = 0; i < count; i++) {
+ node = of_parse_phandle(dev->of_node, "memory-region", i);
+ if (of_device_is_compatible(node, "restricted-dma-pool"))
And you may want to add here an of_device_is_available(node). A platform
that provides the Device Tree firmware and try to support multiple
different SoCs may try to determine if an IOMMU is present, and if it
is, it could be marking the restriced-dma-pool region with a 'status =
"disabled"' property, or any variant of that scheme.
This function is called only when there is no IOMMU present (check in
drivers/of/device.c). I can still add of_device_is_available(node)
here if you think it's helpful.
I believe it is, since boot loader can have a shared Device Tree blob
skeleton and do various adaptations based on the chip (that's what we
do) and adding a status property is much simpler than insertion new
nodes are run time.
This does not seem to be supporting more than one memory region, did not
you want something like instead:
ret = of_reserved_mem_device_init_by_idx(...);
if (ret)
return ret;
Yes. This implement only supports one restriced-dma-pool memory region
with the assumption that there is only one memory region with the
compatible string, restricted-dma-pool, in the dts. IIUC, it's similar
to shared-dma-pool.
Then if here is such a known limitation it should be both documented and
enforced here, you shouldn ot be iterating over all of the phandles that
you find, stop at the first one and issue a warning if count > 1?
--
Florian
On Fri, Jan 15, 2021 at 2:52 AM Florian Fainelli [off-list ref] wrote:
On 1/14/21 1:08 AM, Claire Chang wrote:
quoted
On Wed, Jan 13, 2021 at 7:48 AM Florian Fainelli [off-list ref] wrote:
quoted
On 1/5/21 7:41 PM, Claire Chang wrote:
quoted
If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.
Signed-off-by: Claire Chang <redacted>
---
You could have an early check for count < 0, along with an error
message, if that is deemed useful.
quoted
+ for (i = 0; i < count; i++) {
+ node = of_parse_phandle(dev->of_node, "memory-region", i);
+ if (of_device_is_compatible(node, "restricted-dma-pool"))
And you may want to add here an of_device_is_available(node). A platform
that provides the Device Tree firmware and try to support multiple
different SoCs may try to determine if an IOMMU is present, and if it
is, it could be marking the restriced-dma-pool region with a 'status =
"disabled"' property, or any variant of that scheme.
This function is called only when there is no IOMMU present (check in
drivers/of/device.c). I can still add of_device_is_available(node)
here if you think it's helpful.
I believe it is, since boot loader can have a shared Device Tree blob
skeleton and do various adaptations based on the chip (that's what we
do) and adding a status property is much simpler than insertion new
nodes are run time.
This does not seem to be supporting more than one memory region, did not
you want something like instead:
ret = of_reserved_mem_device_init_by_idx(...);
if (ret)
return ret;
Yes. This implement only supports one restriced-dma-pool memory region
with the assumption that there is only one memory region with the
compatible string, restricted-dma-pool, in the dts. IIUC, it's similar
to shared-dma-pool.
Then if here is such a known limitation it should be both documented and
enforced here, you shouldn ot be iterating over all of the phandles that
you find, stop at the first one and issue a warning if count > 1?
What I have in mind is there might be multiple memory regions, but
only one is for restriced-dma-pool.
Say, if you want a separated region for coherent DMA and only do
streaming DMA in this restriced-dma-pool region, you can add another
reserved-memory node with shared-dma-pool in dts and the current
implementation will try to allocate the memory via
dma_alloc_from_dev_coherent() first (see dma_alloc_attrs() in
/kernel/dma/mapping.c).
Or if you have vendor specific memory region, you can still set up
restriced-dma-pool by adding another reserved-memory node in dts.
Dose this make sense to you? I'll document this for sure.
Hi,
First of all let me say that I am glad that someone is working on a
upstream solution for this issue, would appreciate if you could CC and
Jim Quinlan on subsequent submissions.
On 1/5/21 7:41 PM, Claire Chang wrote:
This series implements mitigations for lack of DMA access control on
systems without an IOMMU, which could result in the DMA accessing the
system memory at unexpected times and/or unexpected addresses, possibly
leading to data leakage or corruption.
For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is
not behind an IOMMU. As PCI-e, by design, gives the device full access to
system memory, a vulnerability in the Wi-Fi firmware could easily escalate
to a full system exploit (remote wifi exploits: [1a], [1b] that shows a
full chain of exploits; [2], [3]).
To mitigate the security concerns, we introduce restricted DMA. Restricted
DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a
specially allocated region and does memory allocation from the same region.
The feature on its own provides a basic level of protection against the DMA
overwriting buffer contents at unexpected times. However, to protect
against general data leakage and system memory corruption, the system needs
to provide a way to restrict the DMA to a predefined memory region (this is
usually done at firmware level, e.g. in ATF on some ARM platforms).
Can you explain how ATF gets involved and to what extent it does help,
besides enforcing a secure region from the ARM CPU's perpsective? Does
the PCIe root complex not have an IOMMU but can somehow be denied access
to a region that is marked NS=0 in the ARM CPU's MMU? If so, that is
still some sort of basic protection that the HW enforces, right?
On Broadcom STB SoCs we have had something similar for a while however
and while we don't have an IOMMU for the PCIe bridge, we do have a a
basic protection mechanism whereby we can configure a region in DRAM to
be PCIe read/write and CPU read/write which then gets used as the PCIe
inbound region for the PCIe EP. By default the PCIe bridge is not
allowed access to DRAM so we must call into a security agent to allow
the PCIe bridge to access the designated DRAM region.
We have done this using a private CMA area region assigned via Device
Tree, assigned with a and requiring the PCIe EP driver to use
dma_alloc_from_contiguous() in order to allocate from this device
private CMA area. The only drawback with that approach is that it
requires knowing how much memory you need up front for buffers and DMA
descriptors that the PCIe EP will need to process. The problem is that
it requires driver modifications and that does not scale over the number
of PCIe EP drivers, some we absolutely do not control, but there is no
need to bounce buffer. Your approach scales better across PCIe EP
drivers however it does require bounce buffering which could be a
performance hit.
Thanks!
--
Florian
On Thu, Jan 7, 2021 at 2:48 AM Florian Fainelli [off-list ref] wrote:
Hi,
First of all let me say that I am glad that someone is working on a
upstream solution for this issue, would appreciate if you could CC and
Jim Quinlan on subsequent submissions.
Sure!
On 1/5/21 7:41 PM, Claire Chang wrote:
quoted
This series implements mitigations for lack of DMA access control on
systems without an IOMMU, which could result in the DMA accessing the
system memory at unexpected times and/or unexpected addresses, possibly
leading to data leakage or corruption.
For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is
not behind an IOMMU. As PCI-e, by design, gives the device full access to
system memory, a vulnerability in the Wi-Fi firmware could easily escalate
to a full system exploit (remote wifi exploits: [1a], [1b] that shows a
full chain of exploits; [2], [3]).
To mitigate the security concerns, we introduce restricted DMA. Restricted
DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a
specially allocated region and does memory allocation from the same region.
The feature on its own provides a basic level of protection against the DMA
overwriting buffer contents at unexpected times. However, to protect
against general data leakage and system memory corruption, the system needs
to provide a way to restrict the DMA to a predefined memory region (this is
usually done at firmware level, e.g. in ATF on some ARM platforms).
Can you explain how ATF gets involved and to what extent it does help,
besides enforcing a secure region from the ARM CPU's perpsective? Does
the PCIe root complex not have an IOMMU but can somehow be denied access
to a region that is marked NS=0 in the ARM CPU's MMU? If so, that is
still some sort of basic protection that the HW enforces, right?
We need the ATF support for memory MPU (memory protection unit).
Restricted DMA (with reserved-memory in dts) makes sure the predefined memory
region is for PCIe DMA only, but we still need MPU to locks down PCIe access to
that specific regions.
On Broadcom STB SoCs we have had something similar for a while however
and while we don't have an IOMMU for the PCIe bridge, we do have a a
basic protection mechanism whereby we can configure a region in DRAM to
be PCIe read/write and CPU read/write which then gets used as the PCIe
inbound region for the PCIe EP. By default the PCIe bridge is not
allowed access to DRAM so we must call into a security agent to allow
the PCIe bridge to access the designated DRAM region.
We have done this using a private CMA area region assigned via Device
Tree, assigned with a and requiring the PCIe EP driver to use
dma_alloc_from_contiguous() in order to allocate from this device
private CMA area. The only drawback with that approach is that it
requires knowing how much memory you need up front for buffers and DMA
descriptors that the PCIe EP will need to process. The problem is that
it requires driver modifications and that does not scale over the number
of PCIe EP drivers, some we absolutely do not control, but there is no
need to bounce buffer. Your approach scales better across PCIe EP
drivers however it does require bounce buffering which could be a
performance hit.
Only the streaming DMA (map/unmap) needs bounce buffering.
I also added alloc/free support in this series
(https://lore.kernel.org/patchwork/patch/1360995/), so dma_direct_alloc() will
try to allocate memory from the predefined memory region.
As for the performance hit, it should be similar to the default swiotlb.
Here are my experiment results. Both SoCs lack IOMMU for PCIe.
PCIe wifi vht80 throughput -
MTK SoC tcp_tx tcp_rx udp_tx udp_rx
w/o Restricted DMA 244.1 134.66 312.56 350.79
w/ Restricted DMA 246.95 136.59 363.21 351.99
Rockchip SoC tcp_tx tcp_rx udp_tx udp_rx
w/o Restricted DMA 237.87 133.86 288.28 361.88
w/ Restricted DMA 256.01 130.95 292.28 353.19
The CPU usage doesn't increase too much either.
Although I didn't measure the CPU usage very precisely, it's ~3% with a single
big core (Cortex-A72) and ~5% with a single small core (Cortex-A53).
Thanks!