| Message ID | 20230221094018.19840-1-zyytlz.wz@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: bttv: fix use after free error due to btv->timeout timer | expand |
Hillf Danton <hdanton@sina.com> 于2023年2月21日周二 19:15写道： > > Feel free to add info like how this fix was tested. Hi Hillf, Sorry for my unclear description. I don't have the device to test. I learn it from someone else's experience. > > > > Fix it by adding del_timer_sync invoking to the remove function. > > > > cpu0 cpu1 > > bttv_probe > > ->timer_setup > > ->bttv_set_dma > > ->mod_timer; > > bttv_remove > > ->kfree(btv); > > ->bttv_irq_timeout > > ->USE btv > > > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > > --- > > drivers/media/pci/bt8xx/bttv-driver.c | 1 + > > drivers/media/pci/bt8xx/bttv-risc.c | 2 +- > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c > > index d40b537f4e98..24ba5729969d 100644 > > --- a/drivers/media/pci/bt8xx/bttv-driver.c > > +++ b/drivers/media/pci/bt8xx/bttv-driver.c > > @@ -4248,6 +4248,7 @@ static void bttv_remove(struct pci_dev *pci_dev) > > > > /* free resources */ > > free_irq(btv->c.pci->irq,btv); > > + del_timer_sync(&btv->timeout); > > iounmap(btv->bt848_mmio); > > release_mem_region(pci_resource_start(btv->c.pci,0), > > pci_resource_len(btv->c.pci,0)); > > diff --git a/drivers/media/pci/bt8xx/bttv-risc.c b/drivers/media/pci/bt8xx/bttv-risc.c > > index 32fa4a7fe76f..ada469198645 100644 > > --- a/drivers/media/pci/bt8xx/bttv-risc.c > > +++ b/drivers/media/pci/bt8xx/bttv-risc.c > > @@ -481,7 +481,7 @@ bttv_set_dma(struct bttv *btv, int override) > > if (btv->curr.frame_irq || btv->loop_irq || btv->cvbi) { > > mod_timer(&btv->timeout, jiffies+BTTV_TIMEOUT); > > } else { > > - del_timer(&btv->timeout); > > + del_timer_sync(&btv->timeout); > > } > > btv->main.cpu[RISC_SLOT_LOOP] = cpu_to_le32(cmd); > > > > -- > > 2.25.1 > > This patch adds chance for deadlock by replacing del_timer() with del_timer_sync(), right? > > cpu 0 cpu 2 > --- --- > bttv_read() bttv_irq_timeout() > bttv_reinit_bt848() > spin_lock_irqsave(&btv->s_lock,flags); > > spin_lock_irqsave(&btv->s_lock,flags); > deadlock; > btv->errors=0; > bttv_set_dma(btv,0); > del_timer_sync(&btv->timeout); Yes, it is. Thanks for pointing that out. Could you please give some advice about the fix? Best regards, Zheng
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c index d40b537f4e98..24ba5729969d 100644 --- a/drivers/media/pci/bt8xx/bttv-driver.c +++ b/drivers/media/pci/bt8xx/bttv-driver.c @@ -4248,6 +4248,7 @@ static void bttv_remove(struct pci_dev *pci_dev) /* free resources */ free_irq(btv->c.pci->irq,btv); + del_timer_sync(&btv->timeout); iounmap(btv->bt848_mmio); release_mem_region(pci_resource_start(btv->c.pci,0), pci_resource_len(btv->c.pci,0)); diff --git a/drivers/media/pci/bt8xx/bttv-risc.c b/drivers/media/pci/bt8xx/bttv-risc.c index 32fa4a7fe76f..ada469198645 100644 --- a/drivers/media/pci/bt8xx/bttv-risc.c +++ b/drivers/media/pci/bt8xx/bttv-risc.c @@ -481,7 +481,7 @@ bttv_set_dma(struct bttv *btv, int override) if (btv->curr.frame_irq || btv->loop_irq || btv->cvbi) { mod_timer(&btv->timeout, jiffies+BTTV_TIMEOUT); } else { - del_timer(&btv->timeout); + del_timer_sync(&btv->timeout); } btv->main.cpu[RISC_SLOT_LOOP] = cpu_to_le32(cmd);
There may be some a race condition between timer function bttv_irq_timeout and bttv_remove. The timer is setup in probe and there is no timer_delete operation in remove function. When it hit kfree btv, the function might still be invoked, which will cause use after free bug. This bug is found by static analysis, it may be false positive. Fix it by adding del_timer_sync invoking to the remove function. cpu0 cpu1 bttv_probe ->timer_setup ->bttv_set_dma ->mod_timer; bttv_remove ->kfree(btv); ->bttv_irq_timeout ->USE btv Signed-off-by: Zheng Wang <zyytlz.wz@163.com> --- drivers/media/pci/bt8xx/bttv-driver.c | 1 + drivers/media/pci/bt8xx/bttv-risc.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)