Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues: - [Critical] An AB-BA deadlock occurs between the V4L2 `VIDIOC_STREAMOFF` ioctl and the workqueue worker `esparser_queue_all_src` over `sess->lock`. - [Critical] A recursive mutex self-deadlock occurs because `core->lock` is manually acquired inside functions already protected by the V4L2 core. - [High] The `esparser_queue_all_src` workqueue thread enters a 100% CPU infinite busy-loop on empty queues, end-of-stream buffers, or hardware FIFO full conditions. - [High] The V4L2 M2M framework permanently stalls due to a leaked job context when `esparser_queue_all_src` exits early or hits loop break conditions without calling `v4l2_m2m_job_finish()`. - [High] Closing any secondary file descriptor unconditionally clears `core->cur_sess`, stripping hardware ownership from the active session and causing permanent DMA memory leaks. Pre-existing issues: - [Critical] An unconditional out-of-bounds heap write occurs when padding artificially small, user-controlled buffers in `esparser_pad_start_code()`. - [Critical] An integer overflow during VP9 sub-frame length parsing allows attackers to bypass size boundaries and trigger an out-of-bounds `memmove` copy. -- --- Patch [12]: [PATCH v7 12/19] media: meson: vdec: Coordinate m2m task execution inside async loop --- commit 8f8ffb4492573d6d82ee209ecb382a9f5fbed9e4 Author: Anand Moon <[email protected]> media: meson: vdec: Coordinate m2m task execution inside async loop This commit introduces proper V4L2 Memory-to-Memory (m2m) scheduler pipeline tracking by handling job finalisation inside the asynchronous esparser workqueue. It defers the scheduling call to v4l2_m2m_job_finish() to execute within the worker routine. > diff --git a/drivers/staging/media/meson/vdec/esparser.c > b/drivers/staging/media/meson/vdec/esparser.c > index b9f36fef4be12..939b239c2af47 100644 > --- a/drivers/staging/media/meson/vdec/esparser.c > +++ b/drivers/staging/media/meson/vdec/esparser.c [ ... ] > @@ -437,6 +438,9 @@ void esparser_queue_all_src(struct work_struct *work) > else > v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE); > > + /* Safely notify the V4L2 core sub-framework */ > + v4l2_m2m_job_finish(core->m2m_dev, sess->m2m_ctx); > + > /* Set tracking flag indicating transaction completion > */ > processed_frame = true; > } [Severity: High] Does this worker thread run into an infinite busy-loop on empty queues or end-of-stream buffers? In esparser_queue_all_src(), the code uses a scoped_guard for the lock: while (1) { ... scoped_guard(mutex, &sess->lock) { if (!vbuf) break; } } Since scoped_guard expands into a hidden for-loop, the break statement only exits the scoped_guard's loop, not the outer while (1) loop. This could trap execution indefinitely, consuming 100% CPU when the queue is empty, hits an EOS buffer, or the hardware FIFO is full. [Severity: High] Could the V4L2 M2M job context permanently leak if the session shuts down? Looking at esparser_queue_all_src(): scoped_guard(mutex, &sess->lock) { /* Safe atomic tracking check: exit loop if session is shutting down */ if (READ_ONCE(sess->should_stop)) return; Returning early directly bypasses the newly added v4l2_m2m_job_finish() call. This could permanently hang the V4L2 memory-to-memory framework because it considers the device busy forever, preventing further decoding jobs. [Severity: Critical] Is there a risk of an AB-BA deadlock between the V4L2 VIDIOC_STREAMOFF ioctl and the workqueue worker? The V4L2 core framework acquires sess->lock during VIDIOC_STREAMOFF and then calls vdec_stop_streaming(), which waits for the active worker thread: drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() { ... cancel_work_sync(&sess->esparser_queue_work); } Concurrently, the worker thread esparser_queue_all_src spins up and blocks attempting to acquire the identical sess->lock: drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() { ... scoped_guard(mutex, &sess->lock) { } This sequence would entirely deadlock the system when stopping the video stream. > diff --git a/drivers/staging/media/meson/vdec/vdec.c > b/drivers/staging/media/meson/vdec/vdec.c > index 0eb39aa6014ee..b3e1d99e8889f 100644 > --- a/drivers/staging/media/meson/vdec/vdec.c > +++ b/drivers/staging/media/meson/vdec/vdec.c > @@ -149,6 +149,17 @@ vdec_queue_recycle(struct amvdec_session *sess, struct > vb2_buffer *vb) > static void vdec_m2m_device_run(void *priv) > { > struct amvdec_session *sess = priv; > + struct amvdec_core *core = sess->core; > + > + if (READ_ONCE(sess->should_stop)) { > + v4l2_m2m_job_finish(core->m2m_dev, sess->m2m_ctx); > + return; > + } > + > + mutex_lock(&core->lock); > + if (!core->cur_sess) > + core->cur_sess = sess; > + mutex_unlock(&core->lock); > > schedule_work(&sess->esparser_queue_work); > } [Severity: Critical] Will acquiring core->lock manually here and in vdec_start_streaming result in a recursive mutex self-deadlock? Because vdev->queue is intentionally left NULL for M2M devices, the V4L2 core ioctl wrapper unconditionally acquires vdev->lock (which is mapped to &core->lock) before delegating execution to the ioctl implementations. The underlying driver callback vdec_start_streaming then attempts to manually acquire it again: drivers/staging/media/meson/vdec/vdec.c:vdec_start_streaming() { ... mutex_lock(&core->lock); if (core->cur_sess && core->cur_sess != sess) { } This would immediately self-deadlock the kernel thread processing VIDIOC_STREAMON or VIDIOC_STREAMOFF. [Severity: High] Does closing a secondary file descriptor unintentionally strip hardware ownership from the active session? In vdec_close(), the state modification is placed outside the active session check block: drivers/staging/media/meson/vdec/vdec.c:vdec_close() { ... /* Safely clear hardware ownership since we were confirmed as the owner */ smp_store_release(&core->cur_sess, NULL); } If a user-space process opens the video node twice, streams on the first session, and then closes the second session, this clears core->cur_sess unconditionally. This destroys the ownership state of the active session, bypassing its cleanup sequence and permanently leaking DMA buffers and canvases. [Severity: Critical] This is a pre-existing issue, but can esparser_pad_start_code() cause an out-of-bounds heap write when handling artificially small buffers? If userspace negotiates a small sizeimage and queues a tiny buffer, the padding function computes pad_size based on ESPARSER_MIN_PACKET_SIZE and executes a memset before verifying the vb2_plane_size: drivers/staging/media/meson/vdec/esparser.c:esparser_pad_start_code() { ... pad_size = ESPARSER_MIN_PACKET_SIZE - payload_size; memset(vaddr + payload_size, 0, pad_size); } if ((payload_size + pad_size + SEARCH_PATTERN_LEN) > vb2_plane_size(vb, 0)) { } This could allow a local user to overwrite memory with zeroes by passing a payload_size of 0, resulting in a 4096-byte memset. [Severity: Critical] This is a pre-existing issue, but might an integer overflow in vp9_update_header() allow attackers to trigger an out-of-bounds memmove? The parser reconstructs frame lengths by bit-shifting byte payloads: drivers/staging/media/meson/vdec/esparser.c:vp9_update_header() { ... frame_size[cur_frame] |= (dp[mag_ptr] << (cur_mag * 8)); } If dp[mag_ptr] is >= 128 and cur_mag is 3, the unsigned 8-bit value promotes to an int and shifts into the sign bit, resulting in a large negative integer. Later, this negative frame_size is passed to memmove(): drivers/staging/media/meson/vdec/esparser.c:vp9_update_header() { ... memmove(fdata + VP9_HEADER_SIZE, old_framedata, framesize); } The negative framesize implicitly converts to a massive size_t, which could corrupt arbitrary kernel memory. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
