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

[Pei Xiao]

在 2024/12/3 15:12, Mukesh Kumar Savaliya 写道:
> Hi Pei, Please Don't top post the comment.
so sorry for that!
> 
> 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 ?
when I grep "cancel_work_sync" -nr driver/,almost no use of this return 
value in driver.

Thanks!
Pei.

>>>>       i3c_master_unregister(&master->base);
>>>>       pm_runtime_disable(master->dev);
>>>
>>>
> 
>