Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Returning `IRQ_HANDLED` for a NULL session in the hardware interrupt handler causes an infinite interrupt storm. Pre-existing issues: - [High] Calling `synchronize_irq()` before stopping hardware interrupts fails to fix the use-after-free race condition. - [High] Unbind operations destroy devm-managed structures and the m2m device while open file descriptors remain, causing UAF on closure. -- --- Patch [4]: [PATCH v7 04/19] media: meson: vdec: Fix use-after-free race between teardown and ISR routines --- commit 7f1533ed7c182234db28cc4195417115cb635bb9 Author: Anand Moon <[email protected]> media: meson: vdec: Fix use-after-free race between teardown and ISR routines This patch attempts to prevent race conditions and null pointer dereferences by synchronizing interrupt service routines during session teardown and file descriptor release phases. It adds synchronize_irq() calls and uses smp_load_acquire() to safely capture the current session in the ISR handlers. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/staging/media/meson/vdec/vdec.c > b/drivers/staging/media/meson/vdec/vdec.c > index 7fc73d5cdebbf..7ae3d5a9dd6ab 100644 > --- a/drivers/staging/media/meson/vdec/vdec.c > +++ b/drivers/staging/media/meson/vdec/vdec.c > @@ -484,6 +484,9 @@ static void vdec_stop_streaming(struct vb2_queue *q) > } > } > > + /* Synchronize and flush pending hardware interrupt service > routines */ > + synchronize_irq(core->vdec_irq); > + [Severity: High] This isn't a bug introduced by this patch, but does calling synchronize_irq() before vdec_poweroff() actually prevent the race condition? Since synchronize_irq() only waits for currently executing handlers to finish and does not disable the IRQ line or prevent new hardware interrupts, it seems new interrupts can still fire immediately after it returns. Because the hardware isn't stopped until vdec_poweroff() runs, could a newly triggered ISR observe a valid session pointer and execute concurrently with the rest of the teardown sequence? Should the hardware be stopped via vdec_poweroff() before synchronizing IRQs? > vdec_poweroff(sess); > vdec_free_canvas(sess); > [ ... ] > @@ -989,6 +992,9 @@ static int vdec_close(struct file *file) > struct amvdec_session *sess = file_to_amvdec_session(file); > struct amvdec_core *core = sess->core; > > + /* Synchronize and flush pending hardware interrupt service routines */ > + synchronize_irq(core->vdec_irq); > + [Severity: High] This is a pre-existing issue, but does the driver handle unbind safely if user-space keeps the video device file open? If the driver is unbound, vdec_remove() executes: vdec_remove() video_unregister_device(core->vdev_dec); v4l2_m2m_release(core->m2m_dev); v4l2_device_unregister(&core->v4l2_dev); Since devres automatically frees the amvdec_core structure upon unbind completion, and video_unregister_device() does not forcefully close open file descriptors, could a later call to vdec_close() dereference the freed core structure? For instance, this newly added synchronize_irq(core->vdec_irq) call would use the freed core pointer. Similarly, v4l2_m2m_ctx_release(sess->m2m_ctx) later in vdec_close() would use the freed core->m2m_dev. > if (!IS_ERR_OR_NULL(sess->recycle_thread)) { > kthread_stop(sess->recycle_thread); > sess->recycle_thread = NULL; [ ... ] > @@ -1038,7 +1044,12 @@ static const struct v4l2_file_operations vdec_fops = { > static irqreturn_t vdec_isr(int irq, void *data) > { > struct amvdec_core *core = data; > - struct amvdec_session *sess = core->cur_sess; > + struct amvdec_session *sess; > + > + /* Secure an atomic acquire snapshot to protect against concurrent > teardown */ > + sess = smp_load_acquire(&core->cur_sess); > + if (!sess) > + return IRQ_HANDLED; [Severity: High] Can returning IRQ_HANDLED here cause an infinite interrupt storm? If the handler evaluates the session to NULL and returns IRQ_HANDLED without acknowledging or clearing the interrupt at the hardware level, the generic IRQ framework will unmask the interrupt line. Since the hardware interrupt source hasn't been cleared, won't the interrupt controller immediately re-trigger the interrupt, resulting in a soft lockup or CPU stall? Should this return IRQ_NONE so the IRQ subsystem can detect it as spurious and eventually disable the IRQ line? > > sess->last_irq_jiffies = get_jiffies_64(); > > @@ -1048,7 +1059,12 @@ static irqreturn_t vdec_isr(int irq, void *data) > static irqreturn_t vdec_threaded_isr(int irq, void *data) > { > struct amvdec_core *core = data; > - struct amvdec_session *sess = core->cur_sess; > + struct amvdec_session *sess; > + > + /* Prevent late-stage threaded interrupts from dereferencing a NULL > session */ > + sess = smp_load_acquire(&core->cur_sess); > + if (!sess) > + return IRQ_HANDLED; [Severity: High] Does returning IRQ_HANDLED here also incorrectly propagate the same interrupt storm behavior? > > return sess->fmt_out->codec_ops->threaded_isr(sess); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
