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 <[email protected]>
---
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
Hillf Danton <[email protected]> 于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 <[email protected]>
> > ---
> > 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