acpi: arm64/iort: Ensure DMA mask does not exceed device limit

When calculating the DMA mask from the address limit provided by the firmware, we add one to the ilog2() of the end address, and pass the result to DMA_BIT_MASK().

For an end address that is not a power-of-2 minus 1, this will result in the mask to be wider than the limit, and cover memory that is not addressable by the device. Instead, we should add 1 to 'end' before taking the log, so that a limit of, say, 0x3fffffff gets translated to a mask of 30, but any value below it gets translated to 29.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/acpi/arm64/iort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/arm64/iort.c +1 -1

index 7d04424189df..aab2f51eff14 100644
@@ -1162,7 +1162,7 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
 		 * firmware.
 		 */
 		end = dmaaddr + size - 1;
-		mask = DMA_BIT_MASK(ilog2(end) + 1);
+		mask = DMA_BIT_MASK(ilog2(end + 1));
 		dev->bus_dma_limit = end;
 		dev->coherent_dma_mask = mask;
 		*dev->dma_mask = mask;

Signature · 1 line

2.17.1

[+Robin]

Quoted 10 lines · Ard Biesheuvel

When calculating the DMA mask from the address limit provided by the firmware, we add one to the ilog2() of the end address, and pass the result to DMA_BIT_MASK().

For an end address that is not a power-of-2 minus 1, this will result in the mask to be wider than the limit, and cover memory that is not addressable by the device. Instead, we should add 1 to 'end' before taking the log, so that a limit of, say, 0x3fffffff gets translated to a mask of 30, but any value below it gets translated to 29.

Quoted 4 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
  drivers/acpi/arm64/iort.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Need Robin's feedback on this - I was looking at:

a7ba70f1787f ("dma-mapping: treat dev->bus_dma_mask as a DMA limit")

I assume current code is *intended* but I shall let Robin comment on this.

Thanks,
Lorenzo

Quoted 16 lines

drivers/acpi/arm64/iort.c +1 -1

index 7d04424189df..aab2f51eff14 100644
@@ -1162,7 +1162,7 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)
  		 * firmware.
  		 */
  		end = dmaaddr + size - 1;
-		mask = DMA_BIT_MASK(ilog2(end) + 1);
+		mask = DMA_BIT_MASK(ilog2(end + 1));
  		dev->bus_dma_limit = end;
  		dev->coherent_dma_mask = mask;
  		*dev->dma_mask = mask;

Signature · 1 line

2.17.1

Quoted 18 lines · Lorenzo Pieralisi

[+Robin]

Quoted 10 lines · Ard Biesheuvel

When calculating the DMA mask from the address limit provided by the firmware, we add one to the ilog2() of the end address, and pass the result to DMA_BIT_MASK().

For an end address that is not a power-of-2 minus 1, this will result in the mask to be wider than the limit, and cover memory that is not addressable by the device. Instead, we should add 1 to 'end' before taking the log, so that a limit of, say, 0x3fffffff gets translated to a mask of 30, but any value below it gets translated to 29.

Quoted 4 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
  drivers/acpi/arm64/iort.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 6 lines

Need Robin's feedback on this - I was looking at:

a7ba70f1787f ("dma-mapping: treat dev->bus_dma_mask as a DMA limit")

I assume current code is *intended* but I shall let Robin comment on this.

The device masks represent what bits the device is capable of driving, so rounding up is the correct and intended behaviour - if the interconnect address map imposes a non-power-of-two limit, say 3.75GB, and the device can physically access all of that, then claiming the device can't drive bit 31 and trying to prevent it from accessing the upper 1.75GB is nonsense.

Although TBH none of this really matters much any more - as long as the limit is set correctly nothing bad will happen, and drivers are expected to replace these default masks anyway. In fact ancient drivers that still don't explicitly set their masks will be assuming the defaults are 32-bit, so replacing them with something potentially wider actually invites a whole other set of problems. In the case of of_dma_configure(), it kept the code that combines (*not* replaces) the default device masks with a limit-based mask because it didn't do any harm, but equally it should now be entirely unnecessary, and confusion like this seems like an argument for finally removing it.