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

[Mukesh Kumar Savaliya]

Thanks Pei !

On 12/3/2024 12:53 PM, Pei Xiao wrote:
> 
> 
> 在 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")
Acked-by: Mukesh Kumar Savaliya <quic_msavaliy at quicinc.com>
>>>>> 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.
> 
Sure, looks fine to me.
> Thanks!
> Pei.
> 
>>>>>       i3c_master_unregister(&master->base);
>>>>>       pm_runtime_disable(master->dev);
>>>>
>>>>
>>
>>