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

[Mukesh Kumar Savaliya]

Hi Pei, Please Don't top post the comment.

On 12/3/2024 12:35 PM, Pei Xiao wrote:
> 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.
Return : true if work was pending, false otherwise
Would like to capture the return and notify/print ?
>>>       i3c_master_unregister(&master->base);
>>>       pm_runtime_disable(master->dev);
>>
>>