[PATCH] i3c: dw: Fix use-after-free in dw_i3c_master driver due to race condition

[Pei Xiao]

Hi,
   I don't thinks so.Here is a description of cancel_work_sync.
   *work is guaranteed to be not pending or executing on any CPU*.


cancel_work_sync — cancel a work and wait for it to finish
Synopsis
bool cancel_work_sync (	struct work_struct * work);

Arguments

work

     the work to cancel

Description

Cancel work and wait for its execution to finish. This function can be 
used even if the work re-queues itself or migrates to another workqueue. 
On return from this function, work is guaranteed to be not pending or 
executing on any CPU.

cancel_work_sync(delayed_work->work) must not be used for 
delayed_work's. Use cancel_delayed_work_sync instead.

The caller must ensure that the workqueue on which work was last queued 
can't be destroyed before this function returns.
Return

true if work was pending, false otherwise.

Thanks!
Pei.
在 2024/12/3 14:49, Mukesh Kumar Savaliya 写道:
> 
> 
> On 11/28/2024 7:29 AM, Pei Xiao wrote:
>> In dw_i3c_common_probe, &master->hj_work is bound with
>> dw_i3c_hj_work. And dw_i3c_master_irq_handler can call
>> dw_i3c_master_irq_handle_ibis function to start the work.
>>
>> If we remove the module which will call dw_i3c_common_remove to
>> make cleanup, it will free master->base through i3c_master_unregister
>> while the work mentioned above will be used. The sequence of operations
>> that may lead to a UAF bug is as follows:
>>
>> CPU0                                      CPU1
>>
>>                                       | dw_i3c_hj_work
>> dw_i3c_common_remove                 |
>> i3c_master_unregister(&master->base) |
>> device_unregister(&master->dev)      |
>> device_release                       |
>> //free master->base                  |
>>                                       | i3c_master_do_daa(&master->base)
>>                                       | //use master->base
>>
>> Fix it by ensuring that the work is canceled before proceeding with
>> the cleanup in dw_i3c_common_remove.
>>
>> Fixes: 1dd728f5d4d4 ("i3c: master: Add driver for Synopsys DesignWare 
>> IP")
>> Signed-off-by: Pei Xiao <xiaopei01 at kylinos.cn>
>> ---
>>   drivers/i3c/master/dw-i3c-master.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/i3c/master/dw-i3c-master.c 
>> b/drivers/i3c/master/dw-i3c-master.c
>> index 8d694672c110..dbcd3984f257 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> @@ -1624,6 +1624,7 @@ EXPORT_SYMBOL_GPL(dw_i3c_common_probe);
>>   void dw_i3c_common_remove(struct dw_i3c_master *master)
>>   {
>> +    cancel_work_sync(&master->hj_work);
> You still need to capture return and ensure that the pending work is 
> really completed or no more pending.
>>       i3c_master_unregister(&master->base);
>>       pm_runtime_disable(master->dev);
> 
>