Messages in this thread | | | Subject | Re: [PATCH v3] iommu: Fix potential use-after-free during probe | From | Vijayanand Jitta <> | Date | Mon, 24 Jan 2022 10:11:38 +0530 |
| |
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 <quic_vjitta@quicinc.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 ? > > 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
| |