Kasan has reported the following use after free on dev->iommu.
when a device probe fails and it is in process of freeing dev->iommu
in dev_iommu_free function, a deferred_probe_work_func runs in parallel
and tries to access dev->iommu->fwspec in of_iommu_configure path thus
causing use after free.
BUG: KASAN: use-after-free in of_iommu_configure+0xb4/0x4a4
Read of size 8 at addr ffffff87a2f1acb8 by task kworker/u16:2/153
Workqueue: events_unbound deferred_probe_work_func
Call trace:
dump_backtrace+0x0/0x33c
show_stack+0x18/0x24
dump_stack_lvl+0x16c/0x1e0
print_address_description+0x84/0x39c
__kasan_report+0x184/0x308
kasan_report+0x50/0x78
__asan_load8+0xc0/0xc4
of_iommu_configure+0xb4/0x4a4
of_dma_configure_id+0x2fc/0x4d4
platform_dma_configure+0x40/0x5c
really_probe+0x1b4/0xb74
driver_probe_device+0x11c/0x228
__device_attach_driver+0x14c/0x304
bus_for_each_drv+0x124/0x1b0
__device_attach+0x25c/0x334
device_initial_probe+0x24/0x34
bus_probe_device+0x78/0x134
deferred_probe_work_func+0x130/0x1a8
process_one_work+0x4c8/0x970
worker_thread+0x5c8/0xaec
kthread+0x1f8/0x220
ret_from_fork+0x10/0x18
Allocated by task 1:
____kasan_kmalloc+0xd4/0x114
__kasan_kmalloc+0x10/0x1c
kmem_cache_alloc_trace+0xe4/0x3d4
__iommu_probe_device+0x90/0x394
probe_iommu_group+0x70/0x9c
bus_for_each_dev+0x11c/0x19c
bus_iommu_probe+0xb8/0x7d4
bus_set_iommu+0xcc/0x13c
arm_smmu_bus_init+0x44/0x130 [arm_smmu]
arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
platform_drv_probe+0xe4/0x13c
really_probe+0x2c8/0xb74
driver_probe_device+0x11c/0x228
device_driver_attach+0xf0/0x16c
__driver_attach+0x80/0x320
bus_for_each_dev+0x11c/0x19c
driver_attach+0x38/0x48
bus_add_driver+0x1dc/0x3a4
driver_register+0x18c/0x244
__platform_driver_register+0x88/0x9c
init_module+0x64/0xff4 [arm_smmu]
do_one_initcall+0x17c/0x2f0
do_init_module+0xe8/0x378
load_module+0x3f80/0x4a40
__se_sys_finit_module+0x1a0/0x1e4
__arm64_sys_finit_module+0x44/0x58
el0_svc_common+0x100/0x264
do_el0_svc+0x38/0xa4
el0_svc+0x20/0x30
el0_sync_handler+0x68/0xac
el0_sync+0x160/0x180
Freed by task 1:
kasan_set_track+0x4c/0x84
kasan_set_free_info+0x28/0x4c
____kasan_slab_free+0x120/0x15c
__kasan_slab_free+0x18/0x28
slab_free_freelist_hook+0x204/0x2fc
kfree+0xfc/0x3a4
__iommu_probe_device+0x284/0x394
probe_iommu_group+0x70/0x9c
bus_for_each_dev+0x11c/0x19c
bus_iommu_probe+0xb8/0x7d4
bus_set_iommu+0xcc/0x13c
arm_smmu_bus_init+0x44/0x130 [arm_smmu]
arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
platform_drv_probe+0xe4/0x13c
really_probe+0x2c8/0xb74
driver_probe_device+0x11c/0x228
device_driver_attach+0xf0/0x16c
__driver_attach+0x80/0x320
bus_for_each_dev+0x11c/0x19c
driver_attach+0x38/0x48
bus_add_driver+0x1dc/0x3a4
driver_register+0x18c/0x244
__platform_driver_register+0x88/0x9c
init_module+0x64/0xff4 [arm_smmu]
do_one_initcall+0x17c/0x2f0
do_init_module+0xe8/0x378
load_module+0x3f80/0x4a40
__se_sys_finit_module+0x1a0/0x1e4
__arm64_sys_finit_module+0x44/0x58
el0_svc_common+0x100/0x264
do_el0_svc+0x38/0xa4
el0_svc+0x20/0x30
el0_sync_handler+0x68/0xac
el0_sync+0x160/0x180
Fix this by taking device_lock during probe_iommu_group.
Signed-off-by: Vijayanand Jitta <[email protected]>
---
drivers/iommu/iommu.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index dd7863e..261792d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1617,7 +1617,7 @@ static int probe_iommu_group(struct device *dev, void *data)
{
struct list_head *group_list = data;
struct iommu_group *group;
- int ret;
+ int ret = 0;
/* Device is probed already if in a group */
group = iommu_group_get(dev);
@@ -1626,9 +1626,13 @@ static int probe_iommu_group(struct device *dev, void *data)
return 0;
}
- ret = __iommu_probe_device(dev, group_list);
- if (ret == -ENODEV)
- ret = 0;
+ ret = device_trylock(dev);
+ if (ret) {
+ ret = __iommu_probe_device(dev, group_list);
+ if (ret == -ENODEV)
+ ret = 0;
+ device_unlock(dev);
+ }
return ret;
}
--
2.7.4
On 2022-01-12 13:13, Vijayanand Jitta wrote:
> Kasan has reported the following use after free on dev->iommu.
> when a device probe fails and it is in process of freeing dev->iommu
> in dev_iommu_free function, a deferred_probe_work_func runs in parallel
> and tries to access dev->iommu->fwspec in of_iommu_configure path thus
> causing use after free.
>
> BUG: KASAN: use-after-free in of_iommu_configure+0xb4/0x4a4
> Read of size 8 at addr ffffff87a2f1acb8 by task kworker/u16:2/153
>
> Workqueue: events_unbound deferred_probe_work_func
> Call trace:
> dump_backtrace+0x0/0x33c
> show_stack+0x18/0x24
> dump_stack_lvl+0x16c/0x1e0
> print_address_description+0x84/0x39c
> __kasan_report+0x184/0x308
> kasan_report+0x50/0x78
> __asan_load8+0xc0/0xc4
> of_iommu_configure+0xb4/0x4a4
> of_dma_configure_id+0x2fc/0x4d4
> platform_dma_configure+0x40/0x5c
> really_probe+0x1b4/0xb74
> driver_probe_device+0x11c/0x228
> __device_attach_driver+0x14c/0x304
> bus_for_each_drv+0x124/0x1b0
> __device_attach+0x25c/0x334
> device_initial_probe+0x24/0x34
> bus_probe_device+0x78/0x134
> deferred_probe_work_func+0x130/0x1a8
> process_one_work+0x4c8/0x970
> worker_thread+0x5c8/0xaec
> kthread+0x1f8/0x220
> ret_from_fork+0x10/0x18
>
> Allocated by task 1:
> ____kasan_kmalloc+0xd4/0x114
> __kasan_kmalloc+0x10/0x1c
> kmem_cache_alloc_trace+0xe4/0x3d4
> __iommu_probe_device+0x90/0x394
> probe_iommu_group+0x70/0x9c
> bus_for_each_dev+0x11c/0x19c
> bus_iommu_probe+0xb8/0x7d4
> bus_set_iommu+0xcc/0x13c
> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
> platform_drv_probe+0xe4/0x13c
> really_probe+0x2c8/0xb74
> driver_probe_device+0x11c/0x228
> device_driver_attach+0xf0/0x16c
> __driver_attach+0x80/0x320
> bus_for_each_dev+0x11c/0x19c
> driver_attach+0x38/0x48
> bus_add_driver+0x1dc/0x3a4
> driver_register+0x18c/0x244
> __platform_driver_register+0x88/0x9c
> init_module+0x64/0xff4 [arm_smmu]
> do_one_initcall+0x17c/0x2f0
> do_init_module+0xe8/0x378
> load_module+0x3f80/0x4a40
> __se_sys_finit_module+0x1a0/0x1e4
> __arm64_sys_finit_module+0x44/0x58
> el0_svc_common+0x100/0x264
> do_el0_svc+0x38/0xa4
> el0_svc+0x20/0x30
> el0_sync_handler+0x68/0xac
> el0_sync+0x160/0x180
>
> Freed by task 1:
> kasan_set_track+0x4c/0x84
> kasan_set_free_info+0x28/0x4c
> ____kasan_slab_free+0x120/0x15c
> __kasan_slab_free+0x18/0x28
> slab_free_freelist_hook+0x204/0x2fc
> kfree+0xfc/0x3a4
> __iommu_probe_device+0x284/0x394
> probe_iommu_group+0x70/0x9c
> bus_for_each_dev+0x11c/0x19c
> bus_iommu_probe+0xb8/0x7d4
> bus_set_iommu+0xcc/0x13c
> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
> platform_drv_probe+0xe4/0x13c
> really_probe+0x2c8/0xb74
> driver_probe_device+0x11c/0x228
> device_driver_attach+0xf0/0x16c
> __driver_attach+0x80/0x320
> bus_for_each_dev+0x11c/0x19c
> driver_attach+0x38/0x48
> bus_add_driver+0x1dc/0x3a4
> driver_register+0x18c/0x244
> __platform_driver_register+0x88/0x9c
> init_module+0x64/0xff4 [arm_smmu]
> do_one_initcall+0x17c/0x2f0
> do_init_module+0xe8/0x378
> load_module+0x3f80/0x4a40
> __se_sys_finit_module+0x1a0/0x1e4
> __arm64_sys_finit_module+0x44/0x58
> el0_svc_common+0x100/0x264
> do_el0_svc+0x38/0xa4
> el0_svc+0x20/0x30
> el0_sync_handler+0x68/0xac
> el0_sync+0x160/0x180
>
> Fix this by taking device_lock during probe_iommu_group.
>
> Signed-off-by: Vijayanand Jitta <[email protected]>
> ---
> drivers/iommu/iommu.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index dd7863e..261792d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1617,7 +1617,7 @@ static int probe_iommu_group(struct device *dev, void *data)
> {
> struct list_head *group_list = data;
> struct iommu_group *group;
> - int ret;
> + int ret = 0;
>
> /* Device is probed already if in a group */
> group = iommu_group_get(dev);
> @@ -1626,9 +1626,13 @@ static int probe_iommu_group(struct device *dev, void *data)
> return 0;
> }
>
> - ret = __iommu_probe_device(dev, group_list);
> - if (ret == -ENODEV)
> - ret = 0;
> + ret = device_trylock(dev);
> + if (ret) {
This doesn't seem right - we can't have a non-deterministic situation
where __iommu_probe_device() may or may not be called depending on what
anyone else might be doing with the device at the same time.
I don't fully understand how __iommu_probe_device() and
of_iommu_configure() can be running for the same device at the same
time, but if that's not a race which can be fixed in its own right, then
I think adding a refcount to dev_iommu would be a more sensible way to
mitigate it.
Robin.
> + ret = __iommu_probe_device(dev, group_list);
> + if (ret == -ENODEV)
> + ret = 0;
> + device_unlock(dev);
> + }
>
> return ret;
> }
On 1/18/2022 7:19 PM, Robin Murphy wrote:
> On 2022-01-12 13:13, Vijayanand Jitta wrote:
>> Kasan has reported the following use after free on dev->iommu.
>> when a device probe fails and it is in process of freeing dev->iommu
>> in dev_iommu_free function, a deferred_probe_work_func runs in parallel
>> and tries to access dev->iommu->fwspec in of_iommu_configure path thus
>> causing use after free.
>>
>> BUG: KASAN: use-after-free in of_iommu_configure+0xb4/0x4a4
>> Read of size 8 at addr ffffff87a2f1acb8 by task kworker/u16:2/153
>>
>> Workqueue: events_unbound deferred_probe_work_func
>> Call trace:
>> dump_backtrace+0x0/0x33c
>> show_stack+0x18/0x24
>> dump_stack_lvl+0x16c/0x1e0
>> print_address_description+0x84/0x39c
>> __kasan_report+0x184/0x308
>> kasan_report+0x50/0x78
>> __asan_load8+0xc0/0xc4
>> of_iommu_configure+0xb4/0x4a4
>> of_dma_configure_id+0x2fc/0x4d4
>> platform_dma_configure+0x40/0x5c
>> really_probe+0x1b4/0xb74
>> driver_probe_device+0x11c/0x228
>> __device_attach_driver+0x14c/0x304
>> bus_for_each_drv+0x124/0x1b0
>> __device_attach+0x25c/0x334
>> device_initial_probe+0x24/0x34
>> bus_probe_device+0x78/0x134
>> deferred_probe_work_func+0x130/0x1a8
>> process_one_work+0x4c8/0x970
>> worker_thread+0x5c8/0xaec
>> kthread+0x1f8/0x220
>> ret_from_fork+0x10/0x18
>>
>> Allocated by task 1:
>> ____kasan_kmalloc+0xd4/0x114
>> __kasan_kmalloc+0x10/0x1c
>> kmem_cache_alloc_trace+0xe4/0x3d4
>> __iommu_probe_device+0x90/0x394
>> probe_iommu_group+0x70/0x9c
>> bus_for_each_dev+0x11c/0x19c
>> bus_iommu_probe+0xb8/0x7d4
>> bus_set_iommu+0xcc/0x13c
>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>> platform_drv_probe+0xe4/0x13c
>> really_probe+0x2c8/0xb74
>> driver_probe_device+0x11c/0x228
>> device_driver_attach+0xf0/0x16c
>> __driver_attach+0x80/0x320
>> bus_for_each_dev+0x11c/0x19c
>> driver_attach+0x38/0x48
>> bus_add_driver+0x1dc/0x3a4
>> driver_register+0x18c/0x244
>> __platform_driver_register+0x88/0x9c
>> init_module+0x64/0xff4 [arm_smmu]
>> do_one_initcall+0x17c/0x2f0
>> do_init_module+0xe8/0x378
>> load_module+0x3f80/0x4a40
>> __se_sys_finit_module+0x1a0/0x1e4
>> __arm64_sys_finit_module+0x44/0x58
>> el0_svc_common+0x100/0x264
>> do_el0_svc+0x38/0xa4
>> el0_svc+0x20/0x30
>> el0_sync_handler+0x68/0xac
>> el0_sync+0x160/0x180
>>
>> Freed by task 1:
>> kasan_set_track+0x4c/0x84
>> kasan_set_free_info+0x28/0x4c
>> ____kasan_slab_free+0x120/0x15c
>> __kasan_slab_free+0x18/0x28
>> slab_free_freelist_hook+0x204/0x2fc
>> kfree+0xfc/0x3a4
>> __iommu_probe_device+0x284/0x394
>> probe_iommu_group+0x70/0x9c
>> bus_for_each_dev+0x11c/0x19c
>> bus_iommu_probe+0xb8/0x7d4
>> bus_set_iommu+0xcc/0x13c
>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>> platform_drv_probe+0xe4/0x13c
>> really_probe+0x2c8/0xb74
>> driver_probe_device+0x11c/0x228
>> device_driver_attach+0xf0/0x16c
>> __driver_attach+0x80/0x320
>> bus_for_each_dev+0x11c/0x19c
>> driver_attach+0x38/0x48
>> bus_add_driver+0x1dc/0x3a4
>> driver_register+0x18c/0x244
>> __platform_driver_register+0x88/0x9c
>> init_module+0x64/0xff4 [arm_smmu]
>> do_one_initcall+0x17c/0x2f0
>> do_init_module+0xe8/0x378
>> load_module+0x3f80/0x4a40
>> __se_sys_finit_module+0x1a0/0x1e4
>> __arm64_sys_finit_module+0x44/0x58
>> el0_svc_common+0x100/0x264
>> do_el0_svc+0x38/0xa4
>> el0_svc+0x20/0x30
>> el0_sync_handler+0x68/0xac
>> el0_sync+0x160/0x180
>>
>> Fix this by taking device_lock during probe_iommu_group.
>>
>> Signed-off-by: Vijayanand Jitta <[email protected]>
>> ---
>> drivers/iommu/iommu.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index dd7863e..261792d 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1617,7 +1617,7 @@ static int probe_iommu_group(struct device *dev,
>> void *data)
>> {
>> struct list_head *group_list = data;
>> struct iommu_group *group;
>> - int ret;
>> + int ret = 0;
>> /* Device is probed already if in a group */
>> group = iommu_group_get(dev);
>> @@ -1626,9 +1626,13 @@ static int probe_iommu_group(struct device
>> *dev, void *data)
>> return 0;
>> }
>> - ret = __iommu_probe_device(dev, group_list);
>> - if (ret == -ENODEV)
>> - ret = 0;
>> + ret = device_trylock(dev);
>> + if (ret) {
>
> This doesn't seem right - we can't have a non-deterministic situation
> where __iommu_probe_device() may or may not be called depending on what
> anyone else might be doing with the device at the same time.
>
> I don't fully understand how __iommu_probe_device() and
> of_iommu_configure() can be running for the same device at the same
> time, but if that's not a race which can be fixed in its own right, then
Thanks for the review comments.
During arm_smmu probe, bus_for_each_dev is called which calls
__iommu_probe_device for each all the devs on that bus.
__iommu_probe_device+0x90/0x394
probe_iommu_group+0x70/0x9c
bus_for_each_dev+0x11c/0x19c
bus_iommu_probe+0xb8/0x7d4
bus_set_iommu+0xcc/0x13c
arm_smmu_bus_init+0x44/0x130 [arm_smmu]
arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
and the deferred probe function is calling of_iommu_configure on the
same dev which is currently in __iommu_probe_device path in this case
thus causing the race.
> I think adding a refcount to dev_iommu would be a more sensible way to
> mitigate it.
Right, Adding refcount for dev_iommu should help , I'll post a new patch
with it.
Thanks,
Vijay
>
> Robin.
>
>> + ret = __iommu_probe_device(dev, group_list);
>> + if (ret == -ENODEV)
>> + ret = 0;
>> + device_unlock(dev);
>> + }
>> return ret;
>> }
On 1/18/2022 9:27 PM, Vijayanand Jitta wrote:
>
>
> On 1/18/2022 7:19 PM, Robin Murphy wrote:
>> On 2022-01-12 13:13, Vijayanand Jitta wrote:
>>> Kasan has reported the following use after free on dev->iommu.
>>> when a device probe fails and it is in process of freeing dev->iommu
>>> in dev_iommu_free function, a deferred_probe_work_func runs in parallel
>>> and tries to access dev->iommu->fwspec in of_iommu_configure path thus
>>> causing use after free.
>>>
>>> BUG: KASAN: use-after-free in of_iommu_configure+0xb4/0x4a4
>>> Read of size 8 at addr ffffff87a2f1acb8 by task kworker/u16:2/153
>>>
>>> Workqueue: events_unbound deferred_probe_work_func
>>> Call trace:
>>> dump_backtrace+0x0/0x33c
>>> show_stack+0x18/0x24
>>> dump_stack_lvl+0x16c/0x1e0
>>> print_address_description+0x84/0x39c
>>> __kasan_report+0x184/0x308
>>> kasan_report+0x50/0x78
>>> __asan_load8+0xc0/0xc4
>>> of_iommu_configure+0xb4/0x4a4
>>> of_dma_configure_id+0x2fc/0x4d4
>>> platform_dma_configure+0x40/0x5c
>>> really_probe+0x1b4/0xb74
>>> driver_probe_device+0x11c/0x228
>>> __device_attach_driver+0x14c/0x304
>>> bus_for_each_drv+0x124/0x1b0
>>> __device_attach+0x25c/0x334
>>> device_initial_probe+0x24/0x34
>>> bus_probe_device+0x78/0x134
>>> deferred_probe_work_func+0x130/0x1a8
>>> process_one_work+0x4c8/0x970
>>> worker_thread+0x5c8/0xaec
>>> kthread+0x1f8/0x220
>>> ret_from_fork+0x10/0x18
>>>
>>> Allocated by task 1:
>>> ____kasan_kmalloc+0xd4/0x114
>>> __kasan_kmalloc+0x10/0x1c
>>> kmem_cache_alloc_trace+0xe4/0x3d4
>>> __iommu_probe_device+0x90/0x394
>>> probe_iommu_group+0x70/0x9c
>>> bus_for_each_dev+0x11c/0x19c
>>> bus_iommu_probe+0xb8/0x7d4
>>> bus_set_iommu+0xcc/0x13c
>>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>>> platform_drv_probe+0xe4/0x13c
>>> really_probe+0x2c8/0xb74
>>> driver_probe_device+0x11c/0x228
>>> device_driver_attach+0xf0/0x16c
>>> __driver_attach+0x80/0x320
>>> bus_for_each_dev+0x11c/0x19c
>>> driver_attach+0x38/0x48
>>> bus_add_driver+0x1dc/0x3a4
>>> driver_register+0x18c/0x244
>>> __platform_driver_register+0x88/0x9c
>>> init_module+0x64/0xff4 [arm_smmu]
>>> do_one_initcall+0x17c/0x2f0
>>> do_init_module+0xe8/0x378
>>> load_module+0x3f80/0x4a40
>>> __se_sys_finit_module+0x1a0/0x1e4
>>> __arm64_sys_finit_module+0x44/0x58
>>> el0_svc_common+0x100/0x264
>>> do_el0_svc+0x38/0xa4
>>> el0_svc+0x20/0x30
>>> el0_sync_handler+0x68/0xac
>>> el0_sync+0x160/0x180
>>>
>>> Freed by task 1:
>>> kasan_set_track+0x4c/0x84
>>> kasan_set_free_info+0x28/0x4c
>>> ____kasan_slab_free+0x120/0x15c
>>> __kasan_slab_free+0x18/0x28
>>> slab_free_freelist_hook+0x204/0x2fc
>>> kfree+0xfc/0x3a4
>>> __iommu_probe_device+0x284/0x394
>>> probe_iommu_group+0x70/0x9c
>>> bus_for_each_dev+0x11c/0x19c
>>> bus_iommu_probe+0xb8/0x7d4
>>> bus_set_iommu+0xcc/0x13c
>>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>>> platform_drv_probe+0xe4/0x13c
>>> really_probe+0x2c8/0xb74
>>> driver_probe_device+0x11c/0x228
>>> device_driver_attach+0xf0/0x16c
>>> __driver_attach+0x80/0x320
>>> bus_for_each_dev+0x11c/0x19c
>>> driver_attach+0x38/0x48
>>> bus_add_driver+0x1dc/0x3a4
>>> driver_register+0x18c/0x244
>>> __platform_driver_register+0x88/0x9c
>>> init_module+0x64/0xff4 [arm_smmu]
>>> do_one_initcall+0x17c/0x2f0
>>> do_init_module+0xe8/0x378
>>> load_module+0x3f80/0x4a40
>>> __se_sys_finit_module+0x1a0/0x1e4
>>> __arm64_sys_finit_module+0x44/0x58
>>> el0_svc_common+0x100/0x264
>>> do_el0_svc+0x38/0xa4
>>> el0_svc+0x20/0x30
>>> el0_sync_handler+0x68/0xac
>>> el0_sync+0x160/0x180
>>>
>>> Fix this by taking device_lock during probe_iommu_group.
>>>
>>> Signed-off-by: Vijayanand Jitta <[email protected]>
>>> ---
>>> drivers/iommu/iommu.c | 12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index dd7863e..261792d 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -1617,7 +1617,7 @@ static int probe_iommu_group(struct device *dev,
>>> void *data)
>>> {
>>> struct list_head *group_list = data;
>>> struct iommu_group *group;
>>> - int ret;
>>> + int ret = 0;
>>> /* Device is probed already if in a group */
>>> group = iommu_group_get(dev);
>>> @@ -1626,9 +1626,13 @@ static int probe_iommu_group(struct device
>>> *dev, void *data)
>>> return 0;
>>> }
>>> - ret = __iommu_probe_device(dev, group_list);
>>> - if (ret == -ENODEV)
>>> - ret = 0;
>>> + ret = device_trylock(dev);
>>> + if (ret) {
>>
>> This doesn't seem right - we can't have a non-deterministic situation
>> where __iommu_probe_device() may or may not be called depending on what
>> anyone else might be doing with the device at the same time.
>>
>> I don't fully understand how __iommu_probe_device() and
>> of_iommu_configure() can be running for the same device at the same
>> time, but if that's not a race which can be fixed in its own right, then
>
> Thanks for the review comments.
>
> During arm_smmu probe, bus_for_each_dev is called which calls
> __iommu_probe_device for each all the devs on that bus.
>
> __iommu_probe_device+0x90/0x394
> probe_iommu_group+0x70/0x9c
> bus_for_each_dev+0x11c/0x19c
> bus_iommu_probe+0xb8/0x7d4
> bus_set_iommu+0xcc/0x13c
> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>
> and the deferred probe function is calling of_iommu_configure on the
> same dev which is currently in __iommu_probe_device path in this case
> thus causing the race.
>
>> I think adding a refcount to dev_iommu would be a more sensible way to
>> mitigate it.
>
> Right, Adding refcount for dev_iommu should help , I'll post a new patch
> with it.
>
I was seeing if refcount would help here, there is some issues if we add
a refcount within struct dev_iommu
Here the race between below two functions
process 1:
static void dev_iommu_free(struct device *dev)
{
iommu_fwspec_free(dev);
kfree(dev->iommu);
dev->iommu = NULL;
}
Process 2:
static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
{
if (dev->iommu)
return dev->iommu->fwspec;
else
return NULL;
}
when process1 is in kfree(dev->iommu) , process2 passes the check of
if(dev->iommu) and later get the use after free error when it accesses
dev->iomm->fwspec.
Even if we add a refcount within dev_iommu and then call dev_iommu_free
when refcount reaches 0, we later can't check this refcount in
dev_iommu_fwspec_get since its already freed with kfree.
Another issue is iommu_fwspec_free which is called within dev_iommu_free
calls dev_iommu_fwspec_get , so this again causes issue with refcount.
So, I was thinking of adding something like a bool var iommu_dev_set
with in struct device itself and we initialize during dev_iommu_get and
set it to zero in dev_iommu_free, rest of the places we just check it.
Any thoughts on this ?
Thanks,
Vijay
> Thanks,
> Vijay
>>
>> Robin.
>>
>>> + ret = __iommu_probe_device(dev, group_list);
>>> + if (ret == -ENODEV)
>>> + ret = 0;
>>> + device_unlock(dev);
>>> + }
>>> return ret;
>>> }
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
On 2022-01-21 07:16, Vijayanand Jitta wrote:
>
>
> On 1/18/2022 9:27 PM, Vijayanand Jitta wrote:
>>
>>
>> On 1/18/2022 7:19 PM, Robin Murphy wrote:
>>> On 2022-01-12 13:13, Vijayanand Jitta wrote:
>>>> Kasan has reported the following use after free on dev->iommu.
>>>> when a device probe fails and it is in process of freeing dev->iommu
>>>> in dev_iommu_free function, a deferred_probe_work_func runs in parallel
>>>> and tries to access dev->iommu->fwspec in of_iommu_configure path thus
>>>> causing use after free.
>>>>
>>>> BUG: KASAN: use-after-free in of_iommu_configure+0xb4/0x4a4
>>>> Read of size 8 at addr ffffff87a2f1acb8 by task kworker/u16:2/153
>>>>
>>>> Workqueue: events_unbound deferred_probe_work_func
>>>> Call trace:
>>>> dump_backtrace+0x0/0x33c
>>>> show_stack+0x18/0x24
>>>> dump_stack_lvl+0x16c/0x1e0
>>>> print_address_description+0x84/0x39c
>>>> __kasan_report+0x184/0x308
>>>> kasan_report+0x50/0x78
>>>> __asan_load8+0xc0/0xc4
>>>> of_iommu_configure+0xb4/0x4a4
>>>> of_dma_configure_id+0x2fc/0x4d4
>>>> platform_dma_configure+0x40/0x5c
>>>> really_probe+0x1b4/0xb74
>>>> driver_probe_device+0x11c/0x228
>>>> __device_attach_driver+0x14c/0x304
>>>> bus_for_each_drv+0x124/0x1b0
>>>> __device_attach+0x25c/0x334
>>>> device_initial_probe+0x24/0x34
>>>> bus_probe_device+0x78/0x134
>>>> deferred_probe_work_func+0x130/0x1a8
>>>> process_one_work+0x4c8/0x970
>>>> worker_thread+0x5c8/0xaec
>>>> kthread+0x1f8/0x220
>>>> ret_from_fork+0x10/0x18
>>>>
>>>> Allocated by task 1:
>>>> ____kasan_kmalloc+0xd4/0x114
>>>> __kasan_kmalloc+0x10/0x1c
>>>> kmem_cache_alloc_trace+0xe4/0x3d4
>>>> __iommu_probe_device+0x90/0x394
>>>> probe_iommu_group+0x70/0x9c
>>>> bus_for_each_dev+0x11c/0x19c
>>>> bus_iommu_probe+0xb8/0x7d4
>>>> bus_set_iommu+0xcc/0x13c
>>>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>>>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>>>> platform_drv_probe+0xe4/0x13c
>>>> really_probe+0x2c8/0xb74
>>>> driver_probe_device+0x11c/0x228
>>>> device_driver_attach+0xf0/0x16c
>>>> __driver_attach+0x80/0x320
>>>> bus_for_each_dev+0x11c/0x19c
>>>> driver_attach+0x38/0x48
>>>> bus_add_driver+0x1dc/0x3a4
>>>> driver_register+0x18c/0x244
>>>> __platform_driver_register+0x88/0x9c
>>>> init_module+0x64/0xff4 [arm_smmu]
>>>> do_one_initcall+0x17c/0x2f0
>>>> do_init_module+0xe8/0x378
>>>> load_module+0x3f80/0x4a40
>>>> __se_sys_finit_module+0x1a0/0x1e4
>>>> __arm64_sys_finit_module+0x44/0x58
>>>> el0_svc_common+0x100/0x264
>>>> do_el0_svc+0x38/0xa4
>>>> el0_svc+0x20/0x30
>>>> el0_sync_handler+0x68/0xac
>>>> el0_sync+0x160/0x180
>>>>
>>>> Freed by task 1:
>>>> kasan_set_track+0x4c/0x84
>>>> kasan_set_free_info+0x28/0x4c
>>>> ____kasan_slab_free+0x120/0x15c
>>>> __kasan_slab_free+0x18/0x28
>>>> slab_free_freelist_hook+0x204/0x2fc
>>>> kfree+0xfc/0x3a4
>>>> __iommu_probe_device+0x284/0x394
>>>> probe_iommu_group+0x70/0x9c
>>>> bus_for_each_dev+0x11c/0x19c
>>>> bus_iommu_probe+0xb8/0x7d4
>>>> bus_set_iommu+0xcc/0x13c
>>>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>>>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>>>> platform_drv_probe+0xe4/0x13c
>>>> really_probe+0x2c8/0xb74
>>>> driver_probe_device+0x11c/0x228
>>>> device_driver_attach+0xf0/0x16c
>>>> __driver_attach+0x80/0x320
>>>> bus_for_each_dev+0x11c/0x19c
>>>> driver_attach+0x38/0x48
>>>> bus_add_driver+0x1dc/0x3a4
>>>> driver_register+0x18c/0x244
>>>> __platform_driver_register+0x88/0x9c
>>>> init_module+0x64/0xff4 [arm_smmu]
>>>> do_one_initcall+0x17c/0x2f0
>>>> do_init_module+0xe8/0x378
>>>> load_module+0x3f80/0x4a40
>>>> __se_sys_finit_module+0x1a0/0x1e4
>>>> __arm64_sys_finit_module+0x44/0x58
>>>> el0_svc_common+0x100/0x264
>>>> do_el0_svc+0x38/0xa4
>>>> el0_svc+0x20/0x30
>>>> el0_sync_handler+0x68/0xac
>>>> el0_sync+0x160/0x180
>>>>
>>>> Fix this by taking device_lock during probe_iommu_group.
>>>>
>>>> Signed-off-by: Vijayanand Jitta <[email protected]>
>>>> ---
>>>> drivers/iommu/iommu.c | 12 ++++++++----
>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index dd7863e..261792d 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -1617,7 +1617,7 @@ static int probe_iommu_group(struct device *dev,
>>>> void *data)
>>>> {
>>>> struct list_head *group_list = data;
>>>> struct iommu_group *group;
>>>> - int ret;
>>>> + int ret = 0;
>>>> /* Device is probed already if in a group */
>>>> group = iommu_group_get(dev);
>>>> @@ -1626,9 +1626,13 @@ static int probe_iommu_group(struct device
>>>> *dev, void *data)
>>>> return 0;
>>>> }
>>>> - ret = __iommu_probe_device(dev, group_list);
>>>> - if (ret == -ENODEV)
>>>> - ret = 0;
>>>> + ret = device_trylock(dev);
>>>> + if (ret) {
>>>
>>> This doesn't seem right - we can't have a non-deterministic situation
>>> where __iommu_probe_device() may or may not be called depending on what
>>> anyone else might be doing with the device at the same time.
>>>
>>> I don't fully understand how __iommu_probe_device() and
>>> of_iommu_configure() can be running for the same device at the same
>>> time, but if that's not a race which can be fixed in its own right, then
>>
>> Thanks for the review comments.
>>
>> During arm_smmu probe, bus_for_each_dev is called which calls
>> __iommu_probe_device for each all the devs on that bus.
>>
>> __iommu_probe_device+0x90/0x394
>> probe_iommu_group+0x70/0x9c
>> bus_for_each_dev+0x11c/0x19c
>> bus_iommu_probe+0xb8/0x7d4
>> bus_set_iommu+0xcc/0x13c
>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>>
>> and the deferred probe function is calling of_iommu_configure on the
>> same dev which is currently in __iommu_probe_device path in this case
>> thus causing the race.
>>
>>> I think adding a refcount to dev_iommu would be a more sensible way to
>>> mitigate it.
>>
>> Right, Adding refcount for dev_iommu should help , I'll post a new patch
>> with it.
>>
>
> I was seeing if refcount would help here, there is some issues if we add
> a refcount within struct dev_iommu
>
> Here the race between below two functions
>
> process 1:
> static void dev_iommu_free(struct device *dev)
> {
> iommu_fwspec_free(dev);
> kfree(dev->iommu);
> dev->iommu = NULL;
> }
>
> Process 2:
> static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
> {
> if (dev->iommu)
> return dev->iommu->fwspec;
> else
> return NULL;
> }
>
>
> when process1 is in kfree(dev->iommu) , process2 passes the check of
> if(dev->iommu) and later get the use after free error when it accesses
> dev->iomm->fwspec.
>
> Even if we add a refcount within dev_iommu and then call dev_iommu_free
> when refcount reaches 0, we later can't check this refcount in
> dev_iommu_fwspec_get since its already freed with kfree.
> Another issue is iommu_fwspec_free which is called within dev_iommu_free
> calls dev_iommu_fwspec_get , so this again causes issue with refcount.
>
> So, I was thinking of adding something like a bool var iommu_dev_set
> with in struct device itself and we initialize during dev_iommu_get and
> set it to zero in dev_iommu_free, rest of the places we just check it.
>
> Any thoughts on this ?
Well, yeah... "adding a refcount to dev_iommu" doesn't mean literally
just bodging an extra variable into code not designed for concurrency,
it was meant to imply "thoroughly redesign the current dev_iommu
interfaces to work in a reference-counted manner which actually
acknowledges concurrent usage". The places that currently call
dev_iommu_free() would still set dev->iommu to NULL, *then* drop the
reference from iommu_probe_device(). There wouldn't even need to be an
iommu_fwspec_free() any more, just an iommu_fwspec_put() that releases
the reference from iommu_fwspec_get(), and so on. Having thought it
through this far, though, there are some fiddly bits, and it worries me
that it might be getting too complex for a quick fix, where the real
problem is that the concurrency shouldn't exist in the first place.
Is just bodging dev_iommu_free() into a more sensible order enough to
hide the problem for now? Strictly it might want a memory barrier in
there, but memory ordering is not what I want to be thinking about at
dinnertime on a Friday :)
Robin
----->8-----
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8b86406b7162..9d58a515709e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -207,9 +207,14 @@ static struct dev_iommu *dev_iommu_get(struct
device *dev)
static void dev_iommu_free(struct device *dev)
{
- iommu_fwspec_free(dev);
- kfree(dev->iommu);
+ struct dev_iommu *param = dev->iommu;
+
dev->iommu = NULL;
+ if (param->fwspec) {
+ fwnode_handle_put(param->fwspec->iommu_fwnode);
+ kfree(param->fwspec);
+ }
+ kfree(param);
}
static int __iommu_probe_device(struct device *dev, struct list_head
*group_list)
@@ -2901,13 +2906,7 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_init);
void iommu_fwspec_free(struct device *dev)
{
- struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-
- if (fwspec) {
- fwnode_handle_put(fwspec->iommu_fwnode);
- kfree(fwspec);
- dev_iommu_fwspec_set(dev, NULL);
- }
+ /*TODO: dev_iommu made this redundant */
}
EXPORT_SYMBOL_GPL(iommu_fwspec_free);
On 1/22/2022 12:50 AM, Robin Murphy wrote:
> On 2022-01-21 07:16, Vijayanand Jitta wrote:
>>
>>
>> On 1/18/2022 9:27 PM, Vijayanand Jitta wrote:
>>>
>>>
>>> On 1/18/2022 7:19 PM, Robin Murphy wrote:
>>>> On 2022-01-12 13:13, Vijayanand Jitta wrote:
>>>>> Kasan has reported the following use after free on dev->iommu.
>>>>> when a device probe fails and it is in process of freeing dev->iommu
>>>>> in dev_iommu_free function, a deferred_probe_work_func runs in
>>>>> parallel
>>>>> and tries to access dev->iommu->fwspec in of_iommu_configure path thus
>>>>> causing use after free.
>>>>>
>>>>> BUG: KASAN: use-after-free in of_iommu_configure+0xb4/0x4a4
>>>>> Read of size 8 at addr ffffff87a2f1acb8 by task kworker/u16:2/153
>>>>>
>>>>> Workqueue: events_unbound deferred_probe_work_func
>>>>> Call trace:
>>>>> dump_backtrace+0x0/0x33c
>>>>> show_stack+0x18/0x24
>>>>> dump_stack_lvl+0x16c/0x1e0
>>>>> print_address_description+0x84/0x39c
>>>>> __kasan_report+0x184/0x308
>>>>> kasan_report+0x50/0x78
>>>>> __asan_load8+0xc0/0xc4
>>>>> of_iommu_configure+0xb4/0x4a4
>>>>> of_dma_configure_id+0x2fc/0x4d4
>>>>> platform_dma_configure+0x40/0x5c
>>>>> really_probe+0x1b4/0xb74
>>>>> driver_probe_device+0x11c/0x228
>>>>> __device_attach_driver+0x14c/0x304
>>>>> bus_for_each_drv+0x124/0x1b0
>>>>> __device_attach+0x25c/0x334
>>>>> device_initial_probe+0x24/0x34
>>>>> bus_probe_device+0x78/0x134
>>>>> deferred_probe_work_func+0x130/0x1a8
>>>>> process_one_work+0x4c8/0x970
>>>>> worker_thread+0x5c8/0xaec
>>>>> kthread+0x1f8/0x220
>>>>> ret_from_fork+0x10/0x18
>>>>>
>>>>> Allocated by task 1:
>>>>> ____kasan_kmalloc+0xd4/0x114
>>>>> __kasan_kmalloc+0x10/0x1c
>>>>> kmem_cache_alloc_trace+0xe4/0x3d4
>>>>> __iommu_probe_device+0x90/0x394
>>>>> probe_iommu_group+0x70/0x9c
>>>>> bus_for_each_dev+0x11c/0x19c
>>>>> bus_iommu_probe+0xb8/0x7d4
>>>>> bus_set_iommu+0xcc/0x13c
>>>>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>>>>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>>>>> platform_drv_probe+0xe4/0x13c
>>>>> really_probe+0x2c8/0xb74
>>>>> driver_probe_device+0x11c/0x228
>>>>> device_driver_attach+0xf0/0x16c
>>>>> __driver_attach+0x80/0x320
>>>>> bus_for_each_dev+0x11c/0x19c
>>>>> driver_attach+0x38/0x48
>>>>> bus_add_driver+0x1dc/0x3a4
>>>>> driver_register+0x18c/0x244
>>>>> __platform_driver_register+0x88/0x9c
>>>>> init_module+0x64/0xff4 [arm_smmu]
>>>>> do_one_initcall+0x17c/0x2f0
>>>>> do_init_module+0xe8/0x378
>>>>> load_module+0x3f80/0x4a40
>>>>> __se_sys_finit_module+0x1a0/0x1e4
>>>>> __arm64_sys_finit_module+0x44/0x58
>>>>> el0_svc_common+0x100/0x264
>>>>> do_el0_svc+0x38/0xa4
>>>>> el0_svc+0x20/0x30
>>>>> el0_sync_handler+0x68/0xac
>>>>> el0_sync+0x160/0x180
>>>>>
>>>>> Freed by task 1:
>>>>> kasan_set_track+0x4c/0x84
>>>>> kasan_set_free_info+0x28/0x4c
>>>>> ____kasan_slab_free+0x120/0x15c
>>>>> __kasan_slab_free+0x18/0x28
>>>>> slab_free_freelist_hook+0x204/0x2fc
>>>>> kfree+0xfc/0x3a4
>>>>> __iommu_probe_device+0x284/0x394
>>>>> probe_iommu_group+0x70/0x9c
>>>>> bus_for_each_dev+0x11c/0x19c
>>>>> bus_iommu_probe+0xb8/0x7d4
>>>>> bus_set_iommu+0xcc/0x13c
>>>>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>>>>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>>>>> platform_drv_probe+0xe4/0x13c
>>>>> really_probe+0x2c8/0xb74
>>>>> driver_probe_device+0x11c/0x228
>>>>> device_driver_attach+0xf0/0x16c
>>>>> __driver_attach+0x80/0x320
>>>>> bus_for_each_dev+0x11c/0x19c
>>>>> driver_attach+0x38/0x48
>>>>> bus_add_driver+0x1dc/0x3a4
>>>>> driver_register+0x18c/0x244
>>>>> __platform_driver_register+0x88/0x9c
>>>>> init_module+0x64/0xff4 [arm_smmu]
>>>>> do_one_initcall+0x17c/0x2f0
>>>>> do_init_module+0xe8/0x378
>>>>> load_module+0x3f80/0x4a40
>>>>> __se_sys_finit_module+0x1a0/0x1e4
>>>>> __arm64_sys_finit_module+0x44/0x58
>>>>> el0_svc_common+0x100/0x264
>>>>> do_el0_svc+0x38/0xa4
>>>>> el0_svc+0x20/0x30
>>>>> el0_sync_handler+0x68/0xac
>>>>> el0_sync+0x160/0x180
>>>>>
>>>>> Fix this by taking device_lock during probe_iommu_group.
>>>>>
>>>>> Signed-off-by: Vijayanand Jitta <[email protected]>
>>>>> ---
>>>>> drivers/iommu/iommu.c | 12 ++++++++----
>>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>>> index dd7863e..261792d 100644
>>>>> --- a/drivers/iommu/iommu.c
>>>>> +++ b/drivers/iommu/iommu.c
>>>>> @@ -1617,7 +1617,7 @@ static int probe_iommu_group(struct device *dev,
>>>>> void *data)
>>>>> {
>>>>> struct list_head *group_list = data;
>>>>> struct iommu_group *group;
>>>>> - int ret;
>>>>> + int ret = 0;
>>>>> /* Device is probed already if in a group */
>>>>> group = iommu_group_get(dev);
>>>>> @@ -1626,9 +1626,13 @@ static int probe_iommu_group(struct device
>>>>> *dev, void *data)
>>>>> return 0;
>>>>> }
>>>>> - ret = __iommu_probe_device(dev, group_list);
>>>>> - if (ret == -ENODEV)
>>>>> - ret = 0;
>>>>> + ret = device_trylock(dev);
>>>>> + if (ret) {
>>>>
>>>> This doesn't seem right - we can't have a non-deterministic situation
>>>> where __iommu_probe_device() may or may not be called depending on what
>>>> anyone else might be doing with the device at the same time.
>>>>
>>>> I don't fully understand how __iommu_probe_device() and
>>>> of_iommu_configure() can be running for the same device at the same
>>>> time, but if that's not a race which can be fixed in its own right,
>>>> then
>>>
>>> Thanks for the review comments.
>>>
>>> During arm_smmu probe, bus_for_each_dev is called which calls
>>> __iommu_probe_device for each all the devs on that bus.
>>>
>>> __iommu_probe_device+0x90/0x394
>>> probe_iommu_group+0x70/0x9c
>>> bus_for_each_dev+0x11c/0x19c
>>> bus_iommu_probe+0xb8/0x7d4
>>> bus_set_iommu+0xcc/0x13c
>>> arm_smmu_bus_init+0x44/0x130 [arm_smmu]
>>> arm_smmu_device_probe+0xb88/0xc54 [arm_smmu]
>>>
>>> and the deferred probe function is calling of_iommu_configure on the
>>> same dev which is currently in __iommu_probe_device path in this case
>>> thus causing the race.
>>>
>>>> I think adding a refcount to dev_iommu would be a more sensible way to
>>>> mitigate it.
>>>
>>> Right, Adding refcount for dev_iommu should help , I'll post a new patch
>>> with it.
>>>
>>
>> I was seeing if refcount would help here, there is some issues if we add
>> a refcount within struct dev_iommu
>>
>> Here the race between below two functions
>>
>> process 1:
>> static void dev_iommu_free(struct device *dev)
>> {
>> iommu_fwspec_free(dev);
>> kfree(dev->iommu);
>> dev->iommu = NULL;
>> }
>>
>> Process 2:
>> static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device
>> *dev)
>> {
>> if (dev->iommu)
>> return dev->iommu->fwspec;
>> else
>> return NULL;
>> }
>>
>>
>> when process1 is in kfree(dev->iommu) , process2 passes the check of
>> if(dev->iommu) and later get the use after free error when it accesses
>> dev->iomm->fwspec.
>>
>> Even if we add a refcount within dev_iommu and then call dev_iommu_free
>> when refcount reaches 0, we later can't check this refcount in
>> dev_iommu_fwspec_get since its already freed with kfree.
>> Another issue is iommu_fwspec_free which is called within dev_iommu_free
>> calls dev_iommu_fwspec_get , so this again causes issue with refcount.
>>
>> So, I was thinking of adding something like a bool var iommu_dev_set
>> with in struct device itself and we initialize during dev_iommu_get and
>> set it to zero in dev_iommu_free, rest of the places we just check it.
>>
>> Any thoughts on this ?
>
> Well, yeah... "adding a refcount to dev_iommu" doesn't mean literally
> just bodging an extra variable into code not designed for concurrency,
> it was meant to imply "thoroughly redesign the current dev_iommu
> interfaces to work in a reference-counted manner which actually
> acknowledges concurrent usage". The places that currently call
> dev_iommu_free() would still set dev->iommu to NULL, *then* drop the
> reference from iommu_probe_device(). There wouldn't even need to be an
> iommu_fwspec_free() any more, just an iommu_fwspec_put() that releases
> the reference from iommu_fwspec_get(), and so on. Having thought it
> through this far, though, there are some fiddly bits, and it worries me
> that it might be getting too complex for a quick fix, where the real
> problem is that the concurrency shouldn't exist in the first place.
>
> Is just bodging dev_iommu_free() into a more sensible order enough to
> hide the problem for now? Strictly it might want a memory barrier in
> there, but memory ordering is not what I want to be thinking about at
> dinnertime on a Friday :)
>
Thanks for the review comments.
I See the below reordering should fix this issue, I would keep
iommu_fwspec_free as is, as it is being exported and called from other
paths aswell. I have sent new patch with it.
Thanks,
Vijay
> Robin
>
> ----->8-----
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8b86406b7162..9d58a515709e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -207,9 +207,14 @@ static struct dev_iommu *dev_iommu_get(struct
> device *dev)
>
> static void dev_iommu_free(struct device *dev)
> {
> - iommu_fwspec_free(dev);
> - kfree(dev->iommu);
> + struct dev_iommu *param = dev->iommu;
> +
> dev->iommu = NULL;
> + if (param->fwspec) {
> + fwnode_handle_put(param->fwspec->iommu_fwnode);
> + kfree(param->fwspec);
> + }
> + kfree(param);
> }
>
> static int __iommu_probe_device(struct device *dev, struct list_head
> *group_list)
> @@ -2901,13 +2906,7 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_init);
>
> void iommu_fwspec_free(struct device *dev)
> {
> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> -
> - if (fwspec) {
> - fwnode_handle_put(fwspec->iommu_fwnode);
> - kfree(fwspec);
> - dev_iommu_fwspec_set(dev, NULL);
> - }
> + /*TODO: dev_iommu made this redundant */
> }
> EXPORT_SYMBOL_GPL(iommu_fwspec_free);
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation