On 09/06/14 15:19, Stefan Weil wrote:
Hi Stefan,
The array regs is declared with IOMMU_NREGS (3) elements and accessed
using IOMMU_CTRL (0) and IOMMU_BASE (8). In most cases, those values
are right shifted before being used as an index which results in indices
0 and 1. In one case, this right shift was missing for IOMMU_BASE which
results in an out-of-bounds write access with index 8.
Ah yes, this is correct. Thanks for finding this!
The patch adds the missing shift operation also for IOMMU_CTRL where
it is needed only for cosmetic reasons.
Fine with me too.
Signed-off-by: Stefan Weil <[email protected]>
---
Any reason why the array is declared with 3 elements when only the first 2
are used?
There is a 3rd register which can be used to flush the IOMMU TLB
translations, although it isn't really meaningful here. In the end I
decided to let the code fall into the "unimplemented" section just so it
would be possible to log any accesses in case it affected some of the
other diagnostic registers.
Regards,
Stefan
hw/pci-host/apb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 1497008..887338e 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -1,4 +1,4 @@
-/*
+re/*
* QEMU Ultrasparc APB PCI host
*
* Copyright (c) 2006 Fabrice Bellard
@@ -333,7 +333,7 @@ static void iommu_config_write(void *opaque, hwaddr addr,
is->regs[IOMMU_CTRL >> 3] &= 0xffffffffULL;
is->regs[IOMMU_CTRL >> 3] |= val << 32;
} else {
- is->regs[IOMMU_CTRL] = val;
+ is->regs[IOMMU_CTRL >> 3] = val;
}
break;
case IOMMU_CTRL + 0x4:
@@ -345,7 +345,7 @@ static void iommu_config_write(void *opaque, hwaddr addr,
is->regs[IOMMU_BASE >> 3] &= 0xffffffffULL;
is->regs[IOMMU_BASE >> 3] |= val << 32;
} else {
- is->regs[IOMMU_BASE] = val;
+ is->regs[IOMMU_BASE >> 3] = val;
}
break;
case IOMMU_BASE + 0x4:
I've just done a test with these changes (minus the typo on the first
line) and Linux still seems fine, so happy for this to be applied.
ATB,
Mark.
