[PATCH] i3c: master: svc: Fix use after free vulnerability in svc_i3c_master Driver Due to Race Condition

[PATCH] i3c: master: svc: Fix use after free vulnerability in svc_i3c_master Driver Due to Race Condition

[Kaixin Wang]

In the svc_i3c_master_probe function, &master->hj_work is bound with
svc_i3c_master_hj_work, &master->ibi_work is bound with
svc_i3c_master_ibi_work. And svc_i3c_master_ibi_work  can start the
hj_work, svc_i3c_master_irq_handler can start the ibi_work.

If we remove the module which will call svc_i3c_master_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

                                    | svc_i3c_master_hj_work
svc_i3c_master_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 svc_i3c_master_remove.

Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
---
 drivers/i3c/master/svc-i3c-master.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 0a68fd1b81d4..e084ba648b4a 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1775,6 +1775,7 @@ static void svc_i3c_master_remove(struct platform_device *pdev)
 {
 	struct svc_i3c_master *master = platform_get_drvdata(pdev);
 
+	cancel_work_sync(&master->hj_work);
 	i3c_master_unregister(&master->base);
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
-- 
2.25.1

Re: [PATCH] i3c: master: svc: Fix use after free vulnerability in svc_i3c_master Driver Due to Race Condition

[Frank Li]

On Wed, Sep 11, 2024 at 11:01:35PM +0800, Kaixin Wang wrote:
> In the svc_i3c_master_probe function, &master->hj_work is bound with
> svc_i3c_master_hj_work, &master->ibi_work is bound with
> svc_i3c_master_ibi_work. And svc_i3c_master_ibi_work  can start the
> hj_work, svc_i3c_master_irq_handler can start the ibi_work.
>
> If we remove the module which will call svc_i3c_master_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
>
>                                     | svc_i3c_master_hj_work
> svc_i3c_master_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 svc_i3c_master_remove.
>
> Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
> ---

Please add fixes tag and cc stable.

Frank


>  drivers/i3c/master/svc-i3c-master.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 0a68fd1b81d4..e084ba648b4a 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1775,6 +1775,7 @@ static void svc_i3c_master_remove(struct platform_device *pdev)
>  {
>  	struct svc_i3c_master *master = platform_get_drvdata(pdev);
>
> +	cancel_work_sync(&master->hj_work);
>  	i3c_master_unregister(&master->base);
>
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> --
> 2.25.1
>

Re: [PATCH] i3c: master: svc: Fix use after free vulnerability in svc_i3c_master Driver Due to Race Condition

[Miquel Raynal]

Hi,

Frank.li@nxp.com wrote on Wed, 11 Sep 2024 11:24:57 -0400:

> On Wed, Sep 11, 2024 at 11:01:35PM +0800, Kaixin Wang wrote:
> > In the svc_i3c_master_probe function, &master->hj_work is bound with
> > svc_i3c_master_hj_work, &master->ibi_work is bound with
> > svc_i3c_master_ibi_work. And svc_i3c_master_ibi_work  can start the
> > hj_work, svc_i3c_master_irq_handler can start the ibi_work.
> >
> > If we remove the module which will call svc_i3c_master_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
> >
> >                                     | svc_i3c_master_hj_work
> > svc_i3c_master_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 svc_i3c_master_remove.
> >
> > Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
> > ---  
> 
> Please add fixes tag and cc stable.

Yes indeed. Otherwise looks good to me once this fixed.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH] i3c: master: svc: Fix use after free vulnerability in svc_i3c_master Driver Due to Race Condition

[Kaixin Wang]

在 2024-09-11 23:24:57，"Frank Li" <Frank.li@nxp.com> 写道：
>On Wed, Sep 11, 2024 at 11:01:35PM +0800, Kaixin Wang wrote:
>> In the svc_i3c_master_probe function, &master->hj_work is bound with
>> svc_i3c_master_hj_work, &master->ibi_work is bound with
>> svc_i3c_master_ibi_work. And svc_i3c_master_ibi_work  can start the
>> hj_work, svc_i3c_master_irq_handler can start the ibi_work.
>>
>> If we remove the module which will call svc_i3c_master_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
>>
>>                                     | svc_i3c_master_hj_work
>> svc_i3c_master_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 svc_i3c_master_remove.
>>
>> Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
>> ---
>
>Please add fixes tag and cc stable.
>
>Frank
>

I will add them in the next version of the patch.

Best regards,
Kaixin Wang

>
>>  drivers/i3c/master/svc-i3c-master.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
>> index 0a68fd1b81d4..e084ba648b4a 100644
>> --- a/drivers/i3c/master/svc-i3c-master.c
>> +++ b/drivers/i3c/master/svc-i3c-master.c
>> @@ -1775,6 +1775,7 @@ static void svc_i3c_master_remove(struct platform_device *pdev)
>>  {
>>  	struct svc_i3c_master *master = platform_get_drvdata(pdev);
>>
>> +	cancel_work_sync(&master->hj_work);
>>  	i3c_master_unregister(&master->base);
>>
>>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>> --
>> 2.25.1
>>
>

Re: [PATCH] i3c: master: svc: Fix use after free vulnerability in svc_i3c_master Driver Due to Race Condition

[Kaixin Wang]

在 2024-09-12 14:57:09，"Miquel Raynal" <miquel.raynal@bootlin.com> 写道：
>Hi,
>
>Frank.li@nxp.com wrote on Wed, 11 Sep 2024 11:24:57 -0400:
>
>> On Wed, Sep 11, 2024 at 11:01:35PM +0800, Kaixin Wang wrote:
>> > In the svc_i3c_master_probe function, &master->hj_work is bound with
>> > svc_i3c_master_hj_work, &master->ibi_work is bound with
>> > svc_i3c_master_ibi_work. And svc_i3c_master_ibi_work  can start the
>> > hj_work, svc_i3c_master_irq_handler can start the ibi_work.
>> >
>> > If we remove the module which will call svc_i3c_master_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
>> >
>> >                                     | svc_i3c_master_hj_work
>> > svc_i3c_master_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 svc_i3c_master_remove.
>> >
>> > Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
>> > ---  
>> 
>> Please add fixes tag and cc stable.
>
>Yes indeed. Otherwise looks good to me once this fixed.
>
>Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
>Thanks,
>Miquèl
>

Thanks for the review!

Best regards,
Kaixin Wang