iommu/arm-smmu: Fix out-of-bounds dereference
Robin Murphy
reviewerPATCH 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 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/iommu/arm-smmu.c +4 -2
@@ -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 for_each_cfg_sme (fw, i, idx) \ - for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i) +#define for_each_cfg_sme (fw, i, idx) \ + for (i = 0; \ + i < fw->num_ids && (idx = __fwspec_cfg(fw)->smendx[i], true); \ + ++i)struct arm_smmu_device {struct device *dev;
Signature · 1 line
2.10.2.dirty
PATCH 1/1 iommu/arm-smmu: Fix out-of-bounds dereference
Quoted 8 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.
Quoted 6 lines
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]>
This patch fixes the KASAN splats as I saw (example below).
With this patch applied, my dmesg is free of errors.
So feel free to add:
Thanks,
Mark.
==================================================================
BUG: KASAN: slab-out-of-bounds in arm_smmu_add_device+0x510/0x960 at addr ffff800935c6e72c
Read of size 2 by task swapper/0/1
CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc3-00005-g8cc374c #79
Hardware name: ARM Juno development board (r1) (DT)
Call trace:
[<ffff20000808b2d8>] dump_backtrace+0x0/0x278
[<ffff20000808b564>] show_stack+0x14/0x20
[<ffff2000084e4e4c>] dump_stack+0xa4/0xc8
[<ffff200008256984>] kasan_object_err+0x24/0x80
[<ffff200008256c40>] kasan_report_error+0x208/0x4d0
[<ffff200008257330>] kasan_report+0x40/0x48
[<ffff200008255624>] __asan_load2+0x84/0x98
[<ffff200008743e80>] arm_smmu_add_device+0x510/0x960
[<ffff20000873acf8>] iommu_bus_notifier+0x120/0x160
[<ffff2000081038e4>] notifier_call_chain+0x6c/0xb8
[<ffff200008103dfc>] __blocking_notifier_call_chain+0x5c/0xa0
[<ffff200008103e54>] blocking_notifier_call_chain+0x14/0x20
[<ffff20000874ef98>] device_add+0x5c8/0x840
[<ffff20000863d574>] amba_device_try_add+0x1cc/0x298
[<ffff20000863d7a8>] amba_device_add+0x20/0x148
[<ffff200008ad4344>] of_platform_bus_create+0x34c/0x428
[<ffff200008ad4554>] of_platform_populate+0x4c/0xb8
[<ffff200009389c9c>] of_platform_default_populate_init+0x78/0x8c
[<ffff200008083a28>] do_one_initcall+0x90/0x1c8
[<ffff200009331038>] kernel_init_freeable+0x280/0x324
[<ffff200008cb5d00>] kernel_init+0x10/0x110
[<ffff200008083680>] ret_from_fork+0x10/0x50
Object at ffff800935c6e700, in cache kmalloc-128 size: 128
Allocated:
PID = 1
[<ffff20000808ac90>] save_stack_trace_tsk+0x0/0x180
[<ffff20000808ae38>] save_stack_trace+0x28/0x38
[<ffff200008255ebc>] kasan_kmalloc+0xdc/0x188
[<ffff200008252780>] __kmalloc+0x148/0x238
[<ffff200008743b38>] arm_smmu_add_device+0x1c8/0x960
[<ffff20000873acf8>] iommu_bus_notifier+0x120/0x160
[<ffff2000081038e4>] notifier_call_chain+0x6c/0xb8
[<ffff200008103dfc>] __blocking_notifier_call_chain+0x5c/0xa0
[<ffff200008103e54>] blocking_notifier_call_chain+0x14/0x20
[<ffff20000874ef98>] device_add+0x5c8/0x840
[<ffff20000863d574>] amba_device_try_add+0x1cc/0x298
[<ffff20000863d7a8>] amba_device_add+0x20/0x148
[<ffff200008ad4344>] of_platform_bus_create+0x34c/0x428
[<ffff200008ad4554>] of_platform_populate+0x4c/0xb8
[<ffff200009389c9c>] of_platform_default_populate_init+0x78/0x8c
[<ffff200008083a28>] do_one_initcall+0x90/0x1c8
[<ffff200009331038>] kernel_init_freeable+0x280/0x324
[<ffff200008cb5d00>] kernel_init+0x10/0x110
[<ffff200008083680>] ret_from_fork+0x10/0x50
Freed:
PID = 0
(stack is not available)
Memory state around the buggy address:
ffff800935c6e600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff800935c6e680: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff800935c6e700: 00 00 00 00 00 04 fc fc fc fc fc fc fc fc fc fc
^
ffff800935c6e780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff800935c6e800: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================PATCH 1/1 iommu/arm-smmu: Fix out-of-bounds dereference
In reply to Robin MurphyQuoted 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 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/iommu/arm-smmu.c +0
-0
drivers/iommu/arm-smmu.c struct arm_smmu_master_cfg { · line 330
Quoted 9 lines
drivers/iommu/arm-smmu.c +4
-2@@ -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 for_each_cfg_sme (fw, i, idx) \ - for (i = 0; idx = __fwspec_cfg(fw)->smendx[i], i < fw->num_ids; ++i) +#define for_each_cfg_sme (fw, i, idx) \ + for (i = 0; \ + i < fw->num_ids && (idx = __fwspec_cfg(fw)->smendx[i], true); \ + ++i)
Urgh, that's vile. Is it worth wrapping that in (yet another) macro, which either returns the index or -ENOENT if i is out of bounds?
Will