arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops
PATCH 1/1 arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops
Patch 22b3c181c6c324a46f71aae806d8ddbe61d25761 ("arm: dma-mapping: limit IOMMU mapping size") added a check for IO address space size. However this patch broke IOMMU initialization for typical platforms initialized from device tree, which get the default IO address space size of 4GiB. This value doesn't fit into size_t and fails a check introduced by that commit resulting in failed dma-mapping/iommu initialization. This patch fixes this issue by adding proper support for full 4GiB address space size.
--- arch/arm/include/asm/dma-iommu.h | 2 +- arch/arm/mm/dma-mapping.c | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-)
arch/arm/include/asm/dma-iommu.h +1 -1
@@ -25,7 +25,7 @@ struct dma_iommu_mapping { };struct dma_iommu_mapping * -arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,size_t size); +arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,u64 size);void arm_iommu_release_mapping (struct dma_iommu_mapping *mapping);
arch/arm/mm/dma-mapping.c +3 -6
@@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {* arm_iommu_attach_device function. */ struct dma_iommu_mapping * -arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,size_t size) +arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,u64 size) {unsigned int bits = size >>PAGE_SHIFT ;unsigned int bitmap_size =BITS_TO_LONGS (bits) *sizeof (long );
@@ -2057,11 +2057,8 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,if (!iommu)return false; -/* -* currently arm_iommu_create_mapping() takes a max of size_t -* for size param. So check this limit for now. -*/ -if (size >SIZE_MAX ) +/* Only 32-bit DMA address space is supported for now */ +if (size >DMA_BIT_MASK (32 ) +1 )return false; mapping =arm_iommu_create_mapping (dev->bus, dma_base, size);
Signature · 1 line
1.9.2
PATCH 1/1 arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops
Hi Marek,
Quoted 10 lines · Marek Szyprowski
Patch
22b3c181c6c324a46f71aae806d8ddbe61d25761("arm: dma-mapping: limit IOMMU mapping size") added a check for IO address space size. However this patch broke IOMMU initialization for typical platforms initialized from device tree, which get the default IO address space size of 4GiB. This value doesn't fit intosize_tand fails a check introduced by that commit resulting in failed dma-mapping/iommu initialization. This patch fixes this issue by adding proper support for full 4GiB address space size.Signed-off-by Marek Szyprowski <[email protected]>
Much nicer than my hack of just passing in size-1, thanks! I'd offer a tested-by for the default 32-bit case, however...
Quoted 23 lines
--- arch/arm/include/asm/dma-iommu.h | 2 +- arch/arm/mm/dma-mapping.c | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-)arch/arm/include/asm/dma-iommu.h +1
-1@@ -25,7 +25,7 @@ struct dma_iommu_mapping { };struct dma_iommu_mapping * -arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,size_t size); +arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,u64 size);void arm_iommu_release_mapping (struct dma_iommu_mapping *mapping);arch/arm/mm/dma-mapping.c +0
-0
arch/arm/mm/dma-mapping.c struct dma_map_ops iommu_coherent_ops = { · line 1883
Quoted 7 lines
arch/arm/mm/dma-mapping.c +1
-1@@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {* arm_iommu_attach_device function. */ struct dma_iommu_mapping * -arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,size_t size) +arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,u64 size) {unsigned int bits = size >>PAGE_SHIFT ;
...doesn't this u64->int conversion now have the potential to truncate (if the device has a large enough DMA mask specified) and end up generating a weirdly small mapping rather than failing outright? Admittedly you'd have to have a 44-bit or larger DMA mask, and I'm not sure what the practical likelihood of seeing that even on LPAE systems is, but still I'm a little uneasy about it.
Robin.
Quoted 16 lines
arch/arm/mm/dma-mapping.c +2
-5unsigned int bitmap_size =BITS_TO_LONGS (bits) *sizeof (long );@@ -2057,11 +2057,8 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,if (!iommu)return false; -/* -* currently arm_iommu_create_mapping() takes a max of size_t -* for size param. So check this limit for now. -*/ -if (size >SIZE_MAX ) +/* Only 32-bit DMA address space is supported for now */ +if (size >DMA_BIT_MASK (32 ) +1 )return false; mapping =arm_iommu_create_mapping (dev->bus, dma_base, size);
PATCH 1/1 arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops
Quoted 46 lines · Robin Murphy
Hi Marek,
Quoted 10 lines · Marek Szyprowski
Patch
22b3c181c6c324a46f71aae806d8ddbe61d25761("arm: dma-mapping: limit IOMMU mapping size") added a check for IO address space size. However this patch broke IOMMU initialization for typical platforms initialized from device tree, which get the default IO address space size of 4GiB. This value doesn't fit intosize_tand fails a check introduced by that commit resulting in failed dma-mapping/iommu initialization. This patch fixes this issue by adding proper support for full 4GiB address space size.Signed-off-by Marek Szyprowski <[email protected]>Much nicer than my hack of just passing in size-1, thanks! I'd offer a tested-by for the default 32-bit case, however...
Quoted 29 lines
--- arch/arm/include/asm/dma-iommu.h | 2 +- arch/arm/mm/dma-mapping.c | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-)arch/arm/include/asm/dma-iommu.h +1
-1@@ -25,7 +25,7 @@ struct dma_iommu_mapping { };struct dma_iommu_mapping * -arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,size_t size); +arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,u64 size);void arm_iommu_release_mapping (struct dma_iommu_mapping *mapping);arch/arm/mm/dma-mapping.c +1
-1@@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {* arm_iommu_attach_device function. */ struct dma_iommu_mapping * -arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,size_t size) +arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,u64 size) {
Quoted 8 lines
arch/arm/mm/dma-mapping.c struct dma_map_ops iommu_coherent_ops = { · line 1883
Quoted 1 line
arch/arm/mm/dma-mapping.c +0
-0unsigned int bits = size >>PAGE_SHIFT ;...doesn't this
u64->intconversion now have the potential to truncate (if the device has a large enough DMA mask specified) and end up generating a weirdly small mapping rather than failing outright? Admittedly you'd have to have a 44-bit or larger DMA mask, and I'm not sure what the practical likelihood of seeing that even on LPAE systems is, but still I'm a little uneasy about it.
...by which I mean we should move the current size check into arm_iommu_create_mapping itself, rather than relying on the caller, because looking at dependent bits of code in isolation can be confusing ;)
Robin.
Quoted 22 lines
Quoted 6 lines
arch/arm/mm/dma-mapping.c +0
-0unsigned int bitmap_size =BITS_TO_LONGS (bits) *sizeof (long );@@ -2057,11 +2057,8 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,if (!iommu)return false;arch/arm/mm/dma-mapping.c static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size, · line 2064
Quoted 10 lines
arch/arm/mm/dma-mapping.c +2
-5-/* -* currently arm_iommu_create_mapping() takes a max of size_t -* for size param. So check this limit for now. -*/ -if (size >SIZE_MAX ) +/* Only 32-bit DMA address space is supported for now */ +if (size >DMA_BIT_MASK (32 ) +1 )return false; mapping =arm_iommu_create_mapping (dev->bus, dma_base, size);_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
PATCH 1/1 arm: dma-mapping: fix off-by-one check in arm_setup_iommu_dma_ops
Hello,
Quoted 62 lines · Robin Murphy
Quoted 52 lines · Robin Murphy
Hi Marek,
Quoted 11 lines · Marek Szyprowski
Patch
22b3c181c6c324a46f71aae806d8ddbe61d25761("arm: dma-mapping:
limit
IOMMU mapping size") added a check for IO address space size. However this patch broke IOMMU initialization for typical platforms initialized from device tree, which get the default IO address space size of 4GiB. This value doesn't fit intosize_tand fails a check introduced by that commit resulting in failed dma-mapping/iommu initialization. This patch fixes this issue by adding proper support for full 4GiB address space size.Signed-off-by Marek Szyprowski <[email protected]>Much nicer than my hack of just passing in size-1, thanks! I'd offer a tested-by for the default 32-bit case, however...
Quoted 34 lines
--- arch/arm/include/asm/dma-iommu.h | 2 +- arch/arm/mm/dma-mapping.c | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-)arch/arm/include/asm/dma-iommu.h +1
-1@@ -25,7 +25,7 @@ struct dma_iommu_mapping { };struct dma_iommu_mapping * -arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base, size_t size); +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size);void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);arch/arm/mm/dma-mapping.c +1
-1@@ -1878,7 +1878,7 @@ struct dma_map_ops iommu_coherent_ops = {* arm_iommu_attach_device function. */ struct dma_iommu_mapping * -arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base, size_t size) +arm_iommu_create_mapping (struct bus_type *bus,dma_addr_t base,u64 size) {Quoted 8 lines
Quoted 1 line
unsigned int bits = size >> PAGE_SHIFT;...doesn't this
u64->intconversion now have the potential to truncate (if the device has a large enough DMA mask specified) and end up generating a weirdly small mapping rather than failing outright? Admittedly you'd have to have a 44-bit or larger DMA mask, and I'm not sure what the practical likelihood of seeing that even on LPAE systems is, but still I'm a little uneasy about it.
Quoted 4 lines
...by which I mean we should move the current size check into
arm_iommu_create_mappingitself, rather than relying on the caller, because looking at dependent bits of code in isolation can be confusing ;)
Okay, no problem. I will move this check to arm_iommu_create_mapping() and the
problem of potential truncating will be solved. This fix is really needed for
v4.1-rcX. For next releases this will be probably rewritten for common code with
arm64 anyway.
Quoted 22 lines
Robin.
Quoted 18 lines
Quoted 17 lines
unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);arch/arm/mm/dma-mapping.c +2
-5@@ -2057,11 +2057,8 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size, if (!iommu) return false; -/* -* currently arm_iommu_create_mapping() takes a max of size_t -* for size param. So check this limit for now. -*/ -if (size >SIZE_MAX ) +/* Only 32-bit DMA address space is supported for now */ +if (size >DMA_BIT_MASK (32 ) +1 )return false;mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland