[PATCH] IB/cm: Fix possible use-after-free in ib_cm_cleanup()

[PATCH] IB/cm: Fix possible use-after-free in ib_cm_cleanup()

[Wang Hai]

This module's remove path calls cancel_delayed_work(). However, that
function does not wait until the work function finishes. This means
that the callback function may still be running after the driver's
remove function has finished, which would result in a use-after-free.

Fix by calling cancel_delayed_work_sync(), which ensures that
the work is properly cancelled, no longer running, and unable
to re-schedule itself.

Fixes: 8575329d4f85 ("IB/cm: Fix timewait crash after module unload")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
 drivers/infiniband/core/cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index c903b74f46a4..ae0af63f3271 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -4508,7 +4508,7 @@ static void __exit ib_cm_cleanup(void)
 
 	spin_lock_irq(&cm.lock);
 	list_for_each_entry(timewait_info, &cm.timewait_list, list)
-		cancel_delayed_work(&timewait_info->work.work);
+		cancel_delayed_work_sync(&timewait_info->work.work);
 	spin_unlock_irq(&cm.lock);
 
 	ib_unregister_client(&cm_client);
-- 
2.25.1

Re: [PATCH] IB/cm: Fix possible use-after-free in ib_cm_cleanup()

[Jason Gunthorpe]

On Wed, Oct 13, 2021 at 05:30:16PM +0800, Wang Hai wrote:
> This module's remove path calls cancel_delayed_work(). However, that
> function does not wait until the work function finishes. This means
> that the callback function may still be running after the driver's
> remove function has finished, which would result in a use-after-free.
> 
> Fix by calling cancel_delayed_work_sync(), which ensures that
> the work is properly cancelled, no longer running, and unable
> to re-schedule itself.
> 
> Fixes: 8575329d4f85 ("IB/cm: Fix timewait crash after module unload")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
>  drivers/infiniband/core/cm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index c903b74f46a4..ae0af63f3271 100644
> +++ b/drivers/infiniband/core/cm.c
> @@ -4508,7 +4508,7 @@ static void __exit ib_cm_cleanup(void)
>  
>  	spin_lock_irq(&cm.lock);
>  	list_for_each_entry(timewait_info, &cm.timewait_list, list)
> -		cancel_delayed_work(&timewait_info->work.work);
> +		cancel_delayed_work_sync(&timewait_info->work.work);
>  	spin_unlock_irq(&cm.lock);

No, this will deadlock:

static int cm_timewait_handler(struct cm_work *work)
{
	struct cm_timewait_info *timewait_info;
	struct cm_id_private *cm_id_priv;

	timewait_info = container_of(work, struct cm_timewait_info, work);
	spin_lock_irq(&cm.lock);
         ^^^^^^^^^^^^

Holds the same lock

What is your bug? The destroy_wq() a few lines below will flush out
all the work so it is already not possible that work can still exist
after the driver's remove function has finished.

Jason

Re: [PATCH] IB/cm: Fix possible use-after-free in ib_cm_cleanup()

[wanghai (M)]

在 2021/10/14 2:24, Jason Gunthorpe 写道:
> On Wed, Oct 13, 2021 at 05:30:16PM +0800, Wang Hai wrote:
>> This module's remove path calls cancel_delayed_work(). However, that
>> function does not wait until the work function finishes. This means
>> that the callback function may still be running after the driver's
>> remove function has finished, which would result in a use-after-free.
>>
>> Fix by calling cancel_delayed_work_sync(), which ensures that
>> the work is properly cancelled, no longer running, and unable
>> to re-schedule itself.
>>
>> Fixes: 8575329d4f85 ("IB/cm: Fix timewait crash after module unload")
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Wang Hai <wanghai38@huawei.com>
>>   drivers/infiniband/core/cm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>> index c903b74f46a4..ae0af63f3271 100644
>> +++ b/drivers/infiniband/core/cm.c
>> @@ -4508,7 +4508,7 @@ static void __exit ib_cm_cleanup(void)
>>   
>>   	spin_lock_irq(&cm.lock);
>>   	list_for_each_entry(timewait_info, &cm.timewait_list, list)
>> -		cancel_delayed_work(&timewait_info->work.work);
>> +		cancel_delayed_work_sync(&timewait_info->work.work);
>>   	spin_unlock_irq(&cm.lock);
> No, this will deadlock:
>
> static int cm_timewait_handler(struct cm_work *work)
> {
> 	struct cm_timewait_info *timewait_info;
> 	struct cm_id_private *cm_id_priv;
>
> 	timewait_info = container_of(work, struct cm_timewait_info, work);
> 	spin_lock_irq(&cm.lock);
>           ^^^^^^^^^^^^
>
> Holds the same lock
>
> What is your bug? The destroy_wq() a few lines below will flush out
> all the work so it is already not possible that work can still exist
> after the driver's remove function has finished.
>
> Jason
> .
Sorry, this is a wrong bugfix, thank you for pointing it out.

I was studying the code here and thought there might be a null
pointer reference problem.

You are right, I didn't take into account destroy_workqueue().
There are no bugs here. sorry for making this problematic patch.

Please ignore this patch.

-- 
Wang Hai