| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <348bccb5-ae73-d8ea-8450-8c0de92ec497@codeaurora.org>
Date: Fri, 21 Jan 2022 12:46:02 +0530
From: Vijayanand Jitta <vjitta@...eaurora.org>
To: Vijayanand Jitta <quic_vjitta@...cinc.com>,
Robin Murphy <robin.murphy@....com>, joro@...tes.org,
will@...nel.org, iommu@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org
Cc: kernel-team@...roid.com
Subject: Re: [PATCH v3] iommu: Fix potential use-after-free during probe
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 <quic_vjitta@...cinc.com>
>>> ---
>>> 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
Powered by blists - more mailing lists