iommu/arm-smmu: Fix out-of-bounds dereference
Robin Murphy
reviewerPATCH v2 1/1 iommu/arm-smmu: Fix out-of-bounds dereference
When we iterate a master's config entries, what we generally care about is the entry's stream map index, rather than the entry index itself, so it's nice to have the iterator automatically assign the former from the latter. Unfortunately, booting with KASAN reveals the oversight that using a simple comma operator results in the entry index being dereferenced before being checked for validity, so we always access one element past the end of the fwspec array.
Flip things around so that the check always happens before the index may be dereferenced.
--- drivers/iommu/arm-smmu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/iommu/arm-smmu.c +3 -1
@@ -324,8 +324,10 @@ struct arm_smmu_master_cfg {#define INVALID_SMENDX -1#define __fwspec_cfg (fw) ((struct arm_smmu_master_cfg *)fw->iommu_priv)#define fwspec_smmu (fw) (__fwspec_cfg(fw)->smmu) +#define fwspec_smendx (fw, i) \ + (i >= fw->num_ids ? INVALID_SMENDX : __fwspec_cfg(fw)->smendx[i])#define for_each_cfg_sme (fw, i, idx) \ - for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i) + for (i = 0; idx = fwspec_smendx(fw, i), i < fw->num_ids; ++i)struct arm_smmu_device {struct device *dev;
Signature · 1 line
2.10.2.dirty
PATCH v2 1/1 iommu/arm-smmu: Fix out-of-bounds dereference
Quoted 23 lines · Robin Murphy
When we iterate a master's config entries, what we generally care about is the entry's stream map index, rather than the entry index itself, so it's nice to have the iterator automatically assign the former from the latter. Unfortunately, booting with KASAN reveals the oversight that using a simple comma operator results in the entry index being dereferenced before being checked for validity, so we always access one element past the end of the fwspec array.
Flip things around so that the check always happens before the index may be dereferenced.
Fixes adfec2e709d2 ("iommu/arm-smmu: Convert to iommu_fwspec")Reported-by Mark Rutland <[email protected]>Signed-off-by Robin Murphy <[email protected]>--- drivers/iommu/arm-smmu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/iommu/arm-smmu.c +0
-0
drivers/iommu/arm-smmu.c struct arm_smmu_master_cfg { · line 330
Quoted 8 lines
drivers/iommu/arm-smmu.c +3
-1@@ -324,8 +324,10 @@ struct arm_smmu_master_cfg {#define INVALID_SMENDX -1#define __fwspec_cfg (fw) ((struct arm_smmu_master_cfg *)fw->iommu_priv)#define fwspec_smmu (fw) (__fwspec_cfg(fw)->smmu) +#define fwspec_smendx (fw, i) \ + (i >= fw->num_ids ? INVALID_SMENDX : __fwspec_cfg(fw)->smendx[i])#define for_each_cfg_sme (fw, i, idx) \ - for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i) + for (i = 0; idx = fwspec_smendx(fw, i), i < fw->num_ids; ++i)
That's certainly more readable:
Joerg, if you haven't sent your fixes pull yet, please could you add this on top? Otherwise, I'll queue it for 4.10, given that I think this only causes a KASAN splat (the out-of-bounds read isn't ever used for anything).
Will