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

[Mukesh Kumar Savaliya]

On 1/9/2025 2:08 PM, Pei Xiao wrote:
> 
> 在 2024/12/3 15:23, 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.
>>
> Is there any issue with this patch?
> 
Looks fine to me as there is no use of return value. And you don't want 
to print for debug purpose i think, hence signing off.
> Thanks!
> 
> Pei.
> 
>>
>>>>>>        i3c_master_unregister(&master->base);
>>>>>>        pm_runtime_disable(master->dev);
>>>>>
>>>>>
>>>
>>>
Acked-by: Mukesh Kumar Savaliya <quic_msavaliy at quicinc.com>