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

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

[Pei Xiao]

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@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);
 	i3c_master_unregister(&master->base);
 
 	pm_runtime_disable(master->dev);
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Pei Xiao]

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@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);
 	i3c_master_unregister(&master->base);
 
 	pm_runtime_disable(master->dev);
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Pei Xiao]

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@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);
 	i3c_master_unregister(&master->base);
 
 	pm_runtime_disable(master->dev);
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Pei Xiao]

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@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);
 	i3c_master_unregister(&master->base);
 
 	pm_runtime_disable(master->dev);
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[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@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);


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [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@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);
> 
> 

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [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@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);
>>
>>


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [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@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);
>>>
>>>
> 
> 

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [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@quicinc.com>
>>>>> Signed-off-by: Pei Xiao <xiaopei01@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);
>>>>
>>>>
>>
>>


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Pei Xiao]

在 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@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?

Thanks!

Pei.

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [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@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@quicinc.com>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c