| Message ID | bfc49c9527be5b513e7ceafeba314ca40a5be4bc.1732703537.git.xiaopei01@kylinos.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | i3c: dw: Fix use-after-free in dw_i3c_master driver due to race condition | expand |
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);
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);
>
>
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); >> >>
在 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); >>> >>> > >
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); >>>> >>>> >> >>
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);
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(+)