Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [Critical] vdec_close() frees session memory before releasing the V4L2 M2M context, triggering a NULL pointer dereference during queue teardown. - [Critical] Unconditional execution of vdec_poweroff() and clearing of core->cur_sess in vdec_close() causes Data Abort panics and cross-session corruption. - [Critical] Unconditional execution of vdec_poweroff() in vdec_close() leads to a double free of codec DMA allocations. - [High] vdec_close() prematurely frees session context and DMA buffers without canceling background workqueues, causing Use-After-Free and DMA corruption. Pre-existing issues: - [High] Flawed logic in vdec_stop_streaming() orphans the recycle thread, leading to Use-After-Free during partial STREAMOFF. - [Medium] The err_poweroff unwinding path in vdec_start_streaming() leaks sess->priv and associated codec assets. -- --- Patch [3]: [PATCH v7 03/19] media: meson: vdec: Fix lifecycle leaks and race conditions in recycle_thread --- commit 41dac138c8515aa623b248b4a110d8a7802b604b Author: Anand Moon <[email protected]> Subject: media: meson: vdec: Fix lifecycle leaks and race conditions in recycle_thread This patch validates the kthread_run return status during recycling thread initialization to prevent memory leaks. It also ensures the thread is safely stopped in both vdec_stop_streaming and vdec_close to prevent kernel panics during teardown. Closes: 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 --- a/drivers/staging/media/meson/vdec/vdec.c +++ b/drivers/staging/media/meson/vdec/vdec.c [ ... ] @@ -353,13 +357,23 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) > schedule_work(&sess->esparser_queue_work); > return 0; > > err_poweroff: > vdec_poweroff(sess); > vdec_free_canvas(sess); > vdec_reset_timestamps(sess); > vdec_reset_bufs_recycle(sess); [Severity: Medium] This is a pre-existing issue, but does the err_poweroff unwinding path leak sess->priv? If kthread_run() fails, execution jumps to the err_poweroff label which unwinds various hardware states but misses calling kfree(sess->priv). When the application retries VIDIOC_STREAMON, the codec's start routine unconditionally allocates a new structure over sess->priv, permanently stranding the previous allocation. > err_free_vififo: > if (sess->vififo_vaddr) { > dma_free_coherent(sess->core->dev, sess->vififo_size, [ ... ] @@ -464,7 +478,10 @@ static void vdec_stop_streaming(struct vb2_queue *q) > if (full_cleanup) { > if ((q->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE || > !sess->streamon_out) && vdec_codec_needs_recycle(sess)) { [Severity: High] This is a pre-existing issue, but does this logic orphan the recycle thread leading to a use-after-free during partial STREAMOFF? If userspace invokes VIDIOC_STREAMOFF exclusively on the capture queue while the output queue remains active, this condition evaluates to false, bypassing the kthread_stop() call. Because old_status == STATUS_RUNNING, full_cleanup evaluates to true, proceeding to dismantle the session context and free sess->priv. The orphaned recycle thread may concurrently awaken and attempt to access this freed memory. > - kthread_stop(sess->recycle_thread); > + if (!IS_ERR_OR_NULL(sess->recycle_thread)) { > + kthread_stop(sess->recycle_thread); > + sess->recycle_thread = NULL; > + } > } [ ... ] @@ -970,6 +987,31 @@ static int vdec_open(struct file *file) > static int vdec_close(struct file *file) > { > struct amvdec_session *sess = file_to_amvdec_session(file); > + struct amvdec_core *core = sess->core; > + > + if (!IS_ERR_OR_NULL(sess->recycle_thread)) { > + kthread_stop(sess->recycle_thread); > + sess->recycle_thread = NULL; > + } > + > + mutex_lock(&core->lock); > + > + vdec_poweroff(sess); > + vdec_free_canvas(sess); > + core->cur_sess = NULL; [Severity: Critical] Can this unconditional invocation of vdec_poweroff() and clearing of core->cur_sess cause Data Abort panics and cross-session corruption? If userspace opens two instances (FDs), starts streaming on instance A (acquiring hardware ownership), and then closes instance B: vdec_close() for instance B will unconditionally call vdec_poweroff(sess) and clear core->cur_sess, without verifying if instance B actually owns the hardware. This forces clock disablements while instance A is actively decoding. When the hardware for instance A fires an interrupt, vdec_isr() will dereference core->cur_sess, which was just wrongfully set to NULL, causing a panic. [Severity: Critical] Will this unconditional vdec_poweroff() call also lead to a double free of codec DMA allocations? If userspace cleanly shuts down streaming via VIDIOC_STREAMOFF, vdec_stop_streaming() executes vdec_poweroff(). This invokes the codec stop callback (like codec_h264_stop) which frees memory buffers but does not set their pointers to NULL. When userspace subsequently calls close(), vdec_close() calls vdec_poweroff() again. The codec stop routine will execute a second time, passing the same non-NULL pointers to dma_free_coherent() resulting in a catastrophic double-free. > + > + if (sess->vififo_vaddr) { > + dma_free_coherent(core->dev, sess->vififo_size, > + sess->vififo_vaddr, sess->vififo_paddr); > + sess->vififo_vaddr = NULL; > + sess->vififo_paddr = 0; > + } > + vdec_reset_timestamps(sess); > + vdec_reset_bufs_recycle(sess); > + kfree(sess->priv); [Severity: High] Should we call cancel_work_sync(&sess->esparser_queue_work) before freeing the session context and DMA buffers here? If vdec_close() is called while the background workqueue is actively processing frames, the DMA buffer (sess->vififo_vaddr) and session structure (sess->priv) are immediately freed. The concurrently executing workqueue continues its operation inside esparser_queue(), dereferencing the freed sess->priv pointer and instructing the hardware to perform DMA transfers into the freshly freed physical address, potentially causing severe memory corruption. > + sess->priv = NULL; > + > + mutex_unlock(&core->lock); > > v4l2_m2m_ctx_release(sess->m2m_ctx); [Severity: Critical] Does this sequence trigger a NULL pointer dereference during queue teardown? If a user closes the file descriptor without calling VIDIOC_STREAMOFF, vdec_close() will free sess->priv and set it to NULL. Immediately after, it calls v4l2_m2m_ctx_release(), which triggers vdec_stop_streaming(). Since sess->status is still STATUS_RUNNING, the driver will execute codec_ops->drain(sess). The codec's drain function unconditionally dereferences sess->priv, which was just set to NULL, causing an immediate panic. > v4l2_fh_del(&sess->fh, file); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
