Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] The vdec_recycle_thread is conditionally skipped from being stopped but its resources are unconditionally freed during a partial STREAMOFF, leading to Use-After-Free and list corruption. - [High] Failing start_streaming with -EBUSY permanently leaks streamon flags, causing state inconsistency and cross-session hardware corruption. Pre-existing issues: - [High] The return value of `kthread_run()` is not checked, leading to a kernel crash when `kthread_stop()` is called on an `ERR_PTR`. - [High] vdec_reset_timestamps accesses the timestamps list without holding ts_spinlock, leading to list corruption. - [High] Missing cancel_work_sync for esparser_queue_work causes UAF and unclocked register access. -- --- Patch [2]: [PATCH v7 02/19] media: meson: vdec: Fix STREAMON / STREAMOFF race conditions and session teardown --- commit c8f8b26e4a120933b5b1d329b3cc6e97ea21778d Author: Anand Moon <[email protected]> Subject: media: meson: vdec: Fix STREAMON / STREAMOFF race conditions and session teardown This commit addresses race conditions during concurrent STREAMON / STREAMOFF calls and dynamic resolution changes. It restructures initialization paths, error handling, and state tracking to prevent hardware state corruption. 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 [ ... ] > @@ -308,9 +303,29 @@ static int vdec_start_streaming(struct vb2_queue *q, > unsigned int count) > } > > if (sess->status == STATUS_RUNNING || > - sess->status == STATUS_NEEDS_RESUME || > - sess->status == STATUS_INIT) > + sess->status == STATUS_NEEDS_RESUME) > return 0; > > + /* > + * Secure the core hardware lock before checking availability > + * and updating session states to prevent STREAMON race conditions. > + */ > + mutex_lock(&core->lock); > + if (core->cur_sess && core->cur_sess != sess) { > + ret = -EBUSY; > + mutex_unlock(&core->lock); > + goto err_unlock_no_hw; > + } [Severity: High] If we jump to err_unlock_no_hw here, doesn't it bypass the err_cleanup_session label that clears the streamon flags? If session B calls VIDIOC_STREAMON on its OUTPUT queue and hits this path, sess->streamon_out will remain 1. Could a subsequent VIDIOC_QBUF from session B cause vdec_vb2_buf_queue to schedule esparser_queue_work, potentially overwriting the active hardware registers of the current session? [ ... ] > sess->sequence_cap = 0; > sess->sequence_out = 0; > + > if (vdec_codec_needs_recycle(sess)) > sess->recycle_thread = kthread_run(vdec_recycle_thread, sess, > "vdec_recycle"); [Severity: High] This is a pre-existing issue, but does this code properly handle thread creation failure? If kthread_run fails due to memory constraints, it returns an ERR_PTR. Later in vdec_stop_streaming, this pointer is passed unconditionally to kthread_stop. Could this cause a kernel panic when kthread_stop attempts to dereference the ERR_PTR? > - sess->status = STATUS_INIT; > - core->cur_sess = sess; > schedule_work(&sess->esparser_queue_work); > return 0; [Severity: High] This is a pre-existing issue, but does closing the file descriptor safely clean up the workqueue scheduled here? When the client queues a buffer and then closes the file descriptor, vdec_close destroys mutexes and frees the amvdec_session memory via kfree without cancelling the pending esparser_queue_work. When the workqueue executes vdec_esparser_queue_work, could it access the freed session memory and attempt to take a destroyed mutex? [ ... ] > @@ -399,30 +424,13 @@ 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)) { > + kthread_stop(sess->recycle_thread); > + } > + > + vdec_poweroff(sess); [Severity: High] If a userspace client starts streaming on both queues and then calls VIDIOC_STREAMOFF on the CAPTURE queue, the newly added condition evaluating q->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or !sess->streamon_out will evaluate to false, skipping kthread_stop. Since full_cleanup is true, the code proceeds to tear down resources via vdec_reset_bufs_recycle and kfree of sess->priv. Could this lead to a use-after-free or list corruption if the recycle thread continues executing while its backing resources are freed? > + vdec_free_canvas(sess); > + > + if (sess->vififo_vaddr) { > + dma_free_coherent(sess->core->dev, sess->vififo_size, > + sess->vififo_vaddr, > sess->vififo_paddr); > + sess->vififo_vaddr = NULL; > + sess->vififo_paddr = 0; > + } > + > + vdec_reset_timestamps(sess); [Severity: High] This is a pre-existing issue, but does vdec_reset_timestamps safely access the timestamps list here? The function iterates and frees elements of the sess->timestamps list without holding ts_spinlock. If a concurrent threaded ISR executes amvdec_dst_buf_done_idx and calls amvdec_remove_ts, it will acquire ts_spinlock and attempt to remove an element. Could this race condition lead to linked list corruption and use-after-free? > + vdec_reset_bufs_recycle(sess); > + core->cur_sess = NULL; > + > + kfree(sess->priv); > + sess->priv = NULL; > + } else { > + if (sess->status == STATUS_NEEDS_RESUME) > + sess->changed_format = 0; > + } > + mutex_unlock(&core->lock); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
