| Message ID | 20240911150135.839946-1-kxwang23@m.fudan.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i3c: master: svc: Fix use after free vulnerability in svc_i3c_master Driver Due to Race Condition | expand |
在 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 >> >
在 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
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);
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(+)