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. MPU in ATF on some ARM platforms [4]).
[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/
[4] https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132
v5:
Rebase on latest linux-next
v4:
- Fix spinlock bad magic
- Use rmem->name for debugfs entry
- Address the comments in v3
v3:
Using only one reserved memory region for both streaming DMA and memory
allocation.
https://lore.kernel.org/patchwork/cover/1360992/
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/
Claire Chang (16):
swiotlb: Fix the type of index
swiotlb: Refactor swiotlb init functions
swiotlb: Refactor swiotlb_create_debugfs
swiotlb: Add DMA_RESTRICTED_POOL
swiotlb: Add restricted DMA pool initialization
swiotlb: Add a new get_io_tlb_mem getter
swiotlb: Update is_swiotlb_buffer to add a struct device argument
swiotlb: Update is_swiotlb_active to add a struct device argument
swiotlb: Bounce data from/to restricted DMA pool if available
swiotlb: Move alloc_size to find_slots
swiotlb: Refactor swiotlb_tbl_unmap_single
dma-direct: Add a new wrapper __dma_direct_free_pages()
swiotlb: Add restricted DMA alloc/free support.
dma-direct: Allocate memory from restricted DMA pool if available
dt-bindings: of: Add restricted DMA pool
of: Add plumbing for restricted DMA pool
.../reserved-memory/reserved-memory.txt | 24 ++
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +-
drivers/iommu/dma-iommu.c | 12 +-
drivers/of/address.c | 25 ++
drivers/of/device.c | 3 +
drivers/of/of_private.h | 5 +
drivers/pci/xen-pcifront.c | 2 +-
drivers/xen/swiotlb-xen.c | 2 +-
include/linux/device.h | 4 +
include/linux/swiotlb.h | 41 ++-
kernel/dma/Kconfig | 14 +
kernel/dma/direct.c | 57 +++--
kernel/dma/direct.h | 9 +-
kernel/dma/swiotlb.c | 242 +++++++++++++-----
15 files changed, 347 insertions(+), 97 deletions(-)
--
2.31.1.368.gbe11c130af-goog
Fix the type of index from unsigned int to int since find_slots() might
return -1.
Fixes: 0774983bc923 ("swiotlb: refactor swiotlb_tbl_map_single")
Signed-off-by: Claire Chang <redacted>
---
kernel/dma/swiotlb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-04-23 07:11:38
On Thu, Apr 22, 2021 at 04:14:53PM +0800, Claire Chang wrote:
Fix the type of index from unsigned int to int since find_slots() might
return -1.
Fixes: 0774983bc923 ("swiotlb: refactor swiotlb_tbl_map_single")
Signed-off-by: Claire Chang <redacted>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
it really should go into 5.12. I'm not sure if Konrad is going to
be able to queue this up due to his vacation, so I'm tempted to just
queue it up in the dma-mapping tree.
Add a new function, swiotlb_init_io_tlb_mem, for the io_tlb_mem struct
initialization to make the code reusable.
Note that we now also call set_memory_decrypted in swiotlb_init_with_tbl.
Signed-off-by: Claire Chang <redacted>
---
kernel/dma/swiotlb.c | 51 ++++++++++++++++++++++----------------------
1 file changed, 25 insertions(+), 26 deletions(-)
@@ -184,16 +205,8 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)if(!mem)panic("%s: Failed to allocate %zu bytes align=0x%lx\n",__func__,alloc_size,PAGE_SIZE);-mem->nslabs=nslabs;-mem->start=__pa(tlb);-mem->end=mem->start+bytes;-mem->index=0;-spin_lock_init(&mem->lock);-for(i=0;i<mem->nslabs;i++){-mem->slots[i].list=IO_TLB_SEGSIZE-io_tlb_offset(i);-mem->slots[i].orig_addr=INVALID_PHYS_ADDR;-mem->slots[i].alloc_size=0;-}++swiotlb_init_io_tlb_mem(mem,__pa(tlb),nslabs,false);io_tlb_default_mem=mem;if(verbose)
@@ -295,20 +307,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)if(!mem)return-ENOMEM;-mem->nslabs=nslabs;-mem->start=virt_to_phys(tlb);-mem->end=mem->start+bytes;-mem->index=0;-mem->late_alloc=1;-spin_lock_init(&mem->lock);-for(i=0;i<mem->nslabs;i++){-mem->slots[i].list=IO_TLB_SEGSIZE-io_tlb_offset(i);-mem->slots[i].orig_addr=INVALID_PHYS_ADDR;-mem->slots[i].alloc_size=0;-}--set_memory_decrypted((unsignedlong)tlb,bytes>>PAGE_SHIFT);-memset(tlb,0,bytes);+swiotlb_init_io_tlb_mem(mem,virt_to_phys(tlb),nslabs,true);io_tlb_default_mem=mem;swiotlb_print_info();
--
I'm not 100% sure if set_memory_decrypted is safe to call in
swiotlb_init_with_tbl, but I didn't hit any issue booting with this.
2.31.1.368.gbe11c130af-goog
Split the debugfs creation to make the code reusable for supporting
different bounce buffer pools, e.g. restricted DMA pool.
Signed-off-by: Claire Chang <redacted>
---
kernel/dma/swiotlb.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
@@ -80,6 +80,20 @@ config SWIOTLBboolselectNEED_DMA_MAP_STATE+configDMA_RESTRICTED_POOL+bool"DMA Restricted Pool"+depends onOF&&OF_RESERVED_MEM+selectSWIOTLB+help+ThisenablessupportforrestrictedDMApoolswhichprovidealevelof+DMAmemoryprotectiononsystemswithlimitedhardwareprotection+capabilities,suchasthoselackinganIOMMU.++Formoreinformationsee+<Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt>+and<kernel/dma/swiotlb.c>.+Ifunsure,say"n".+## Should be selected if we can mmap non-coherent mappings to userspace.# The only thing that is really required is a way to set an uncached bit
@@ -681,3 +688,76 @@ static int __init swiotlb_create_default_debugfs(void)late_initcall(swiotlb_create_default_debugfs);#endif++#ifdef CONFIG_DMA_RESTRICTED_POOL+staticintrmem_swiotlb_device_init(structreserved_mem*rmem,+structdevice*dev)+{+structio_tlb_mem*mem=rmem->priv;+unsignedlongnslabs=rmem->size>>IO_TLB_SHIFT;++if(dev->dma_io_tlb_mem)+return0;++/* Since multiple devices can share the same pool, the private data,+*io_tlb_memstruct,willbeinitializedbythefirstdeviceattached+*toit.+*/+if(!mem){+mem=kzalloc(struct_size(mem,slots,nslabs),GFP_KERNEL);+if(!mem)+return-ENOMEM;+#ifdef CONFIG_ARM+if(!PageHighMem(pfn_to_page(PHYS_PFN(rmem->base)))){+kfree(mem);+return-EINVAL;+}+#endif /* CONFIG_ARM */+swiotlb_init_io_tlb_mem(mem,rmem->base,nslabs,false);++rmem->priv=mem;+}++#ifdef CONFIG_DEBUG_FS+if(!io_tlb_default_mem->debugfs)+io_tlb_default_mem->debugfs=+debugfs_create_dir("swiotlb",NULL);++swiotlb_create_debugfs(mem,rmem->name,io_tlb_default_mem->debugfs);+#endif /* CONFIG_DEBUG_FS */++dev->dma_io_tlb_mem=mem;++return0;+}++staticvoidrmem_swiotlb_device_release(structreserved_mem*rmem,+structdevice*dev)+{+if(dev)+dev->dma_io_tlb_mem=NULL;+}++staticconststructreserved_mem_opsrmem_swiotlb_ops={+.device_init=rmem_swiotlb_device_init,+.device_release=rmem_swiotlb_device_release,+};++staticint__initrmem_swiotlb_setup(structreserved_mem*rmem)+{+unsignedlongnode=rmem->fdt_node;++if(of_get_flat_dt_prop(node,"reusable",NULL)||+of_get_flat_dt_prop(node,"linux,cma-default",NULL)||+of_get_flat_dt_prop(node,"linux,dma-default",NULL)||+of_get_flat_dt_prop(node,"no-map",NULL))+return-EINVAL;++rmem->ops=&rmem_swiotlb_ops;+pr_info("Reserved memory: created device swiotlb memory pool at %pa, size %ld MiB\n",+&rmem->base,(unsignedlong)rmem->size/SZ_1M);+return0;+}++RESERVEDMEM_OF_DECLARE(dma,"restricted-dma-pool",rmem_swiotlb_setup);+#endif /* CONFIG_DMA_RESTRICTED_POOL */
@@ -681,3 +688,76 @@ static int __init swiotlb_create_default_debugfs(void)late_initcall(swiotlb_create_default_debugfs);#endif++#ifdef CONFIG_DMA_RESTRICTED_POOL+staticintrmem_swiotlb_device_init(structreserved_mem*rmem,+structdevice*dev)+{+structio_tlb_mem*mem=rmem->priv;+unsignedlongnslabs=rmem->size>>IO_TLB_SHIFT;++if(dev->dma_io_tlb_mem)+return0;++/* Since multiple devices can share the same pool, the private data,+*io_tlb_memstruct,willbeinitializedbythefirstdeviceattached+*toit.+*/+if(!mem){+mem=kzalloc(struct_size(mem,slots,nslabs),GFP_KERNEL);+if(!mem)+return-ENOMEM;+#ifdef CONFIG_ARM+if(!PageHighMem(pfn_to_page(PHYS_PFN(rmem->base)))){+kfree(mem);+return-EINVAL;+}+#endif /* CONFIG_ARM */+swiotlb_init_io_tlb_mem(mem,rmem->base,nslabs,false);++rmem->priv=mem;+}++#ifdef CONFIG_DEBUG_FS+if(!io_tlb_default_mem->debugfs)+io_tlb_default_mem->debugfs=+debugfs_create_dir("swiotlb",NULL);
At this point it's possible for io_tlb_default_mem to be NULL, leading
to a splat.
But even then if it's not and we have the situation where debugfs==NULL
then the debugfs_create_dir() here will cause a subsequent attempt in
swiotlb_create_debugfs() to fail (directory already exists) leading to
mem->debugfs being assigned an error value. I suspect the creation of
the debugfs directory needs to be separated from io_tlb_default_mem
being set.
Other than that I gave this series a go with our prototype of Arm's
Confidential Computer Architecture[1] - since the majority of the
guest's memory is protected from the host the restricted DMA pool allows
(only) a small area to be shared with the host.
After fixing (well hacking round) the above it all seems to be working
fine with virtio drivers.
Thanks,
Steve
[1]
https://www.arm.com/why-arm/architecture/security-features/arm-confidential-compute-architecture
@@ -681,3 +688,76 @@ static int __init swiotlb_create_default_debugfs(void)late_initcall(swiotlb_create_default_debugfs);#endif++#ifdef CONFIG_DMA_RESTRICTED_POOL+staticintrmem_swiotlb_device_init(structreserved_mem*rmem,+structdevice*dev)+{+structio_tlb_mem*mem=rmem->priv;+unsignedlongnslabs=rmem->size>>IO_TLB_SHIFT;++if(dev->dma_io_tlb_mem)+return0;++/* Since multiple devices can share the same pool, the private data,+*io_tlb_memstruct,willbeinitializedbythefirstdeviceattached+*toit.+*/+if(!mem){+mem=kzalloc(struct_size(mem,slots,nslabs),GFP_KERNEL);+if(!mem)+return-ENOMEM;+#ifdef CONFIG_ARM+if(!PageHighMem(pfn_to_page(PHYS_PFN(rmem->base)))){+kfree(mem);+return-EINVAL;+}+#endif /* CONFIG_ARM */+swiotlb_init_io_tlb_mem(mem,rmem->base,nslabs,false);++rmem->priv=mem;+}++#ifdef CONFIG_DEBUG_FS+if(!io_tlb_default_mem->debugfs)+io_tlb_default_mem->debugfs=+debugfs_create_dir("swiotlb",NULL);
At this point it's possible for io_tlb_default_mem to be NULL, leading
to a splat.
Thanks for pointing this out.
But even then if it's not and we have the situation where debugfs==NULL
then the debugfs_create_dir() here will cause a subsequent attempt in
swiotlb_create_debugfs() to fail (directory already exists) leading to
mem->debugfs being assigned an error value. I suspect the creation of
the debugfs directory needs to be separated from io_tlb_default_mem
being set.
debugfs creation should move into the if (!mem) {...} above to avoid
duplication.
I think having a separated struct dentry pointer for the default
debugfs should be enough?
if (!debugfs)
debugfs = debugfs_create_dir("swiotlb", NULL);
swiotlb_create_debugfs(mem, rmem->name, debugfs);
Other than that I gave this series a go with our prototype of Arm's
Confidential Computer Architecture[1] - since the majority of the
guest's memory is protected from the host the restricted DMA pool allows
(only) a small area to be shared with the host.
After fixing (well hacking round) the above it all seems to be working
fine with virtio drivers.
Thanks,
Steve
[1]
https://www.arm.com/why-arm/architecture/security-features/arm-confidential-compute-architecture
From: Steven Price <steven.price@arm.com> Date: 2021-04-28 09:50:40
On 26/04/2021 17:37, Claire Chang wrote:
On Fri, Apr 23, 2021 at 7:34 PM Steven Price [off-list ref] wrote:
[...]
quoted
But even then if it's not and we have the situation where debugfs==NULL
then the debugfs_create_dir() here will cause a subsequent attempt in
swiotlb_create_debugfs() to fail (directory already exists) leading to
mem->debugfs being assigned an error value. I suspect the creation of
the debugfs directory needs to be separated from io_tlb_default_mem
being set.
debugfs creation should move into the if (!mem) {...} above to avoid
duplication.
I think having a separated struct dentry pointer for the default
debugfs should be enough?
if (!debugfs)
debugfs = debugfs_create_dir("swiotlb", NULL);
swiotlb_create_debugfs(mem, rmem->name, debugfs);
Yes that looks like a good solution to me. Although I'd name the
variable something a bit more descriptive than just "debugfs" e.g.
"debugfs_dir" or "debugfs_root".
Thanks,
Steve
Add a new getter, get_io_tlb_mem, to help select the io_tlb_mem struct.
The restricted DMA pool is preferred if available.
Signed-off-by: Claire Chang <redacted>
---
include/linux/swiotlb.h | 11 +++++++++++
1 file changed, 11 insertions(+)
@@ -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;
@@ -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))
I wonder if it's worth trying to fold these other conditions into
is_swiotlb_active() itself? I'm not entirely sure what matters for Xen,
but for the other cases it seems like they probably only care about
whether bouncing may occur for their particular device or not (possibly
they want to be using dma_max_mapping_size() now anyway - TBH I'm
struggling to make sense of what the swiotlb_max_segment business is
supposed to mean).
Otherwise, patch #9 will need to touch here as well to make sure that
per-device forced bouncing is reflected correctly.
Robin.
@@ -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))
I wonder if it's worth trying to fold these other conditions into
is_swiotlb_active() itself? I'm not entirely sure what matters for Xen,
but for the other cases it seems like they probably only care about
whether bouncing may occur for their particular device or not (possibly
they want to be using dma_max_mapping_size() now anyway - TBH I'm
struggling to make sense of what the swiotlb_max_segment business is
supposed to mean).
I think leaving those conditions outside of is_swiotlb_active() might
help avoid confusion with is_dev_swiotlb_force() in patch #9? We need
is_dev_swiotlb_force() only because the restricted DMA pool supports
memory allocation, but the default swiotlb doesn't.
Otherwise, patch #9 will need to touch here as well to make sure that
per-device forced bouncing is reflected correctly.
You're right. Otherwise, is_dev_swiotlb_force is needed here.
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 lock down the memory access, e.g., MPU.
Note that is_dev_swiotlb_force doesn't check if
swiotlb_force == SWIOTLB_FORCE. Otherwise the memory allocation behavior
with default swiotlb will be changed by the following patche
("dma-direct: Allocate memory from restricted DMA pool if available").
Signed-off-by: Claire Chang <redacted>
---
include/linux/swiotlb.h | 13 +++++++++++++
kernel/dma/direct.h | 3 ++-
kernel/dma/swiotlb.c | 8 ++++----
3 files changed, 19 insertions(+), 5 deletions(-)
Add a new function, release_slots, to make the code reusable for supporting
different bounce buffer pools, e.g. restricted DMA pool.
Signed-off-by: Claire Chang <redacted>
---
kernel/dma/swiotlb.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)
Add a new wrapper __dma_direct_free_pages() that will be useful later
for swiotlb_free().
Signed-off-by: Claire Chang <redacted>
---
kernel/dma/direct.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
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 lock down the memory access, e.g., MPU.
Signed-off-by: Claire Chang <redacted>
---
kernel/dma/direct.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
@@ -172,7 +186,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,if(IS_ENABLED(CONFIG_DMA_COHERENT_POOL)&&!gfpflags_allow_blocking(gfp)&&(force_dma_unencrypted(dev)||-(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&!dev_is_dma_coherent(dev))))+(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&+!dev_is_dma_coherent(dev)))&&+!is_dev_swiotlb_force(dev))returndma_direct_alloc_from_pool(dev,size,dma_handle,gfp);/* we always manually zero the memory once we are done */
@@ -253,15 +269,15 @@ void dma_direct_free(struct device *dev, size_t size,unsignedintpage_order=get_order(size);if((attrs&DMA_ATTR_NO_KERNEL_MAPPING)&&-!force_dma_unencrypted(dev)){+!force_dma_unencrypted(dev)&&!is_dev_swiotlb_force(dev)){/* cpu_addr is a struct page cookie, not a kernel address */dma_free_contiguous(dev,cpu_addr,size);return;}if(!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED)&&-!IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&-!dev_is_dma_coherent(dev)){+!IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&!dev_is_dma_coherent(dev)&&+!is_dev_swiotlb_force(dev)){arch_dma_free(dev,size,cpu_addr,dma_addr,attrs);return;}
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-04-23 13:46:38
On 2021-04-22 09:15, Claire Chang wrote:
quoted hunk
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 lock down the memory access, e.g., MPU.
Signed-off-by: Claire Chang <redacted>
---
kernel/dma/direct.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
@@ -172,7 +186,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,if(IS_ENABLED(CONFIG_DMA_COHERENT_POOL)&&!gfpflags_allow_blocking(gfp)&&(force_dma_unencrypted(dev)||-(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&!dev_is_dma_coherent(dev))))+(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&+!dev_is_dma_coherent(dev)))&&+!is_dev_swiotlb_force(dev))returndma_direct_alloc_from_pool(dev,size,dma_handle,gfp);/* we always manually zero the memory once we are done */
@@ -253,15 +269,15 @@ void dma_direct_free(struct device *dev, size_t size,unsignedintpage_order=get_order(size);if((attrs&DMA_ATTR_NO_KERNEL_MAPPING)&&-!force_dma_unencrypted(dev)){+!force_dma_unencrypted(dev)&&!is_dev_swiotlb_force(dev)){/* cpu_addr is a struct page cookie, not a kernel address */dma_free_contiguous(dev,cpu_addr,size);return;}if(!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED)&&-!IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&-!dev_is_dma_coherent(dev)){+!IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&!dev_is_dma_coherent(dev)&&+!is_dev_swiotlb_force(dev)){arch_dma_free(dev,size,cpu_addr,dma_addr,attrs);return;}
Wait, this seems broken for non-coherent devices - in that case we need
to return a non-cacheable address, but we can't simply fall through into
the remapping path below in GFP_ATOMIC context. That's why we need the
atomic pool concept in the first place :/
Unless I've overlooked something, we're still using the regular
cacheable linear map address of the dma_io_tlb_mem buffer, no?
Robin.
On Fri, Apr 23, 2021 at 9:46 PM Robin Murphy [off-list ref] wrote:
On 2021-04-22 09:15, Claire Chang wrote:
quoted
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 lock down the memory access, e.g., MPU.
Signed-off-by: Claire Chang <redacted>
---
kernel/dma/direct.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
@@ -172,7 +186,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,if(IS_ENABLED(CONFIG_DMA_COHERENT_POOL)&&!gfpflags_allow_blocking(gfp)&&(force_dma_unencrypted(dev)||-(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&!dev_is_dma_coherent(dev))))+(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&+!dev_is_dma_coherent(dev)))&&+!is_dev_swiotlb_force(dev))returndma_direct_alloc_from_pool(dev,size,dma_handle,gfp);/* we always manually zero the memory once we are done */
@@ -253,15 +269,15 @@ void dma_direct_free(struct device *dev, size_t size,unsignedintpage_order=get_order(size);if((attrs&DMA_ATTR_NO_KERNEL_MAPPING)&&-!force_dma_unencrypted(dev)){+!force_dma_unencrypted(dev)&&!is_dev_swiotlb_force(dev)){/* cpu_addr is a struct page cookie, not a kernel address */dma_free_contiguous(dev,cpu_addr,size);return;}if(!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED)&&-!IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&-!dev_is_dma_coherent(dev)){+!IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&!dev_is_dma_coherent(dev)&&+!is_dev_swiotlb_force(dev)){arch_dma_free(dev,size,cpu_addr,dma_addr,attrs);return;}
Wait, this seems broken for non-coherent devices - in that case we need
to return a non-cacheable address, but we can't simply fall through into
the remapping path below in GFP_ATOMIC context. That's why we need the
atomic pool concept in the first place :/
Sorry for the late reply. I'm not very familiar with this. I wonder if
the memory returned here must be coherent. If yes, could we say for
this case, one must set up another device coherent pool
(shared-dma-pool) and go with dma_alloc_from_dev_coherent()[1]?
[1] https://elixir.bootlin.com/linux/v5.12/source/kernel/dma/mapping.c#L435
Unless I've overlooked something, we're still using the regular
cacheable linear map address of the dma_io_tlb_mem buffer, no?
Robin.
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 reserved-memory node.
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 lock down+ the memory access, e.g., MPU. - vendor specific string in the form <vendor>,[<device>-]<usage> no-map (optional) - empty property - Indicates the operating system must not create a virtual mapping
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 | 25 +++++++++++++++++++++++++
drivers/of/device.c | 3 +++
drivers/of/of_private.h | 5 +++++
3 files changed, 33 insertions(+)
@@ -1109,6 +1110,30 @@ bool of_dma_is_coherent(struct device_node *np)}EXPORT_SYMBOL_GPL(of_dma_is_coherent);+intof_dma_set_restricted_buffer(structdevice*dev)+{+structdevice_node*node;+intcount,i;++if(!dev->of_node)+return0;++count=of_property_count_elems_of_size(dev->of_node,"memory-region",+sizeof(phandle));+for(i=0;i<count;i++){+node=of_parse_phandle(dev->of_node,"memory-region",i);+/* There might be multiple memory regions, but only one+*restriced-dma-poolregionisallowed.+*/+if(of_device_is_compatible(node,"restricted-dma-pool")&&+of_device_is_available(node))+returnof_reserved_mem_device_init_by_idx(+dev,dev->of_node,i);+}++return0;+}+/***of_mmio_is_nonposted-Checkifdeviceusesnon-postedMMIO*@np:devicenode
On Thu, Apr 22, 2021 at 4:17 PM Claire Chang [off-list ref] wrote:
quoted hunk
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 | 25 +++++++++++++++++++++++++
drivers/of/device.c | 3 +++
drivers/of/of_private.h | 5 +++++
3 files changed, 33 insertions(+)
@@ -1109,6 +1110,30 @@ bool of_dma_is_coherent(struct device_node *np)}EXPORT_SYMBOL_GPL(of_dma_is_coherent);+intof_dma_set_restricted_buffer(structdevice*dev)+{+structdevice_node*node;+intcount,i;++if(!dev->of_node)+return0;++count=of_property_count_elems_of_size(dev->of_node,"memory-region",+sizeof(phandle));+for(i=0;i<count;i++){+node=of_parse_phandle(dev->of_node,"memory-region",i);+/* There might be multiple memory regions, but only one+*restriced-dma-poolregionisallowed.+*/+if(of_device_is_compatible(node,"restricted-dma-pool")&&+of_device_is_available(node))+returnof_reserved_mem_device_init_by_idx(+dev,dev->of_node,i);+}++return0;+}+/***of_mmio_is_nonposted-Checkifdeviceusesnon-postedMMIO*@np:devicenode
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-04-23 13:36:57
On 2021-04-22 09:15, Claire Chang wrote:
quoted hunk
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 | 25 +++++++++++++++++++++++++
drivers/of/device.c | 3 +++
drivers/of/of_private.h | 5 +++++
3 files changed, 33 insertions(+)
@@ -1109,6 +1110,30 @@ bool of_dma_is_coherent(struct device_node *np)}EXPORT_SYMBOL_GPL(of_dma_is_coherent);+intof_dma_set_restricted_buffer(structdevice*dev)+{+structdevice_node*node;+intcount,i;++if(!dev->of_node)+return0;++count=of_property_count_elems_of_size(dev->of_node,"memory-region",+sizeof(phandle));+for(i=0;i<count;i++){+node=of_parse_phandle(dev->of_node,"memory-region",i);+/* There might be multiple memory regions, but only one+*restriced-dma-poolregionisallowed.+*/
What's the use-case for having multiple regions if the restricted pool
is by definition the only one accessible?
Robin.
On Fri, Apr 23, 2021 at 9:35 PM Robin Murphy [off-list ref] wrote:
On 2021-04-22 09:15, 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>
---
drivers/of/address.c | 25 +++++++++++++++++++++++++
drivers/of/device.c | 3 +++
drivers/of/of_private.h | 5 +++++
3 files changed, 33 insertions(+)
@@ -1109,6 +1110,30 @@ bool of_dma_is_coherent(struct device_node *np)}EXPORT_SYMBOL_GPL(of_dma_is_coherent);+intof_dma_set_restricted_buffer(structdevice*dev)+{+structdevice_node*node;+intcount,i;++if(!dev->of_node)+return0;++count=of_property_count_elems_of_size(dev->of_node,"memory-region",+sizeof(phandle));+for(i=0;i<count;i++){+node=of_parse_phandle(dev->of_node,"memory-region",i);+/* There might be multiple memory regions, but only one+*restriced-dma-poolregionisallowed.+*/
What's the use-case for having multiple regions if the restricted pool
is by definition the only one accessible?
There might be a device coherent pool (shared-dma-pool) and
dma_alloc_attrs might allocate memory from that pool [1].
I'm not sure if it makes sense to have another device coherent pool
while using restricted DMA pool though.
[1] https://elixir.bootlin.com/linux/v5.12/source/kernel/dma/mapping.c#L435