Re: [PATCH 06/11] media: amlogic: vdec: add MJPEG decoding

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] media: amlogic: vdec: premature M2M job completion in mjpeg_job_done()
- [Critical] media: amlogic: vdec: unsynchronized teardown and HW stop in aml_mjpeg_exit()
- [High] media: amlogic: vdec: timeout race with IRQ thread in aml_mjpeg_dec_run()
- [High] media: amlogic: vdec: NULL dereference on vb2_plane_vaddr() in mjpeg.c
- [Medium] media: amlogic: vdec: missing V4L2_PIX_FMT_MJPEG in aml_s4_video_formats
- [Medium] media: amlogic: vdec: unrestricted CAPTURE formats for MJPEG negotiation
--

commit 03dd48f8ca05ff47abd88fc4288b36619393b97e
Author: Christian Stefan Kövér-Draxl [off-list ref]

media: amlogic: vdec: add MJPEG decoding

This commit adds Motion-JPEG decoding support on the Amlogic legacy VDEC
core, including format registration, firmware table entry, and the MJPEG
codec implementation.

[Severity: Medium]
The commit description states that this patch decodes V4L2_PIX_FMT_MJPEG
frames, and mjpeg.c describes Motion-JPEG decoding, but aml_s4_video_formats
only registers V4L2_PIX_FMT_JPEG.

Because aml_vdec_get_video_fmt() in drivers/media/platform/amlogic/vdec/aml_vdec.c
matches strictly on fmt->fourcc == format:

    for (k = 0; k < dev->pvdec_data->num_fmts; k++) {
        fmt = &dev->pvdec_data->dec_fmt[k];
        if (fmt->fourcc == format)
            return fmt;
    }

clients requesting V4L2_PIX_FMT_MJPEG via VIDIOC_ENUM_FMT,
VIDIOC_ENUM_FRAMESIZES, or VIDIOC_S_FMT will not find the format, and
vdec_s_fmt_output() will fall back to the default H.264 format.

Also, the commit message mentions that the planar 4:2:0 CAPTURE format is
exposed by this patch, whereas V4L2_PIX_FMT_YUV420 was already present in
aml_s4_video_formats from commit a516ebbbab5a.

Should V4L2_PIX_FMT_MJPEG be added to aml_s4_video_formats as well?

[ ... ]

[ ... ]

[Severity: Critical]
Does calling v4l2_m2m_buf_done_and_job_finish() here in
mjpeg_threaded_isr() -> mjpeg_job_done() while aml_mjpeg_dec_run() is still
sleeping in wait_event_timeout() cause use-after-free and hardware races?

When m2mops_vdec_device_run() in drivers/media/platform/amlogic/vdec/aml_vdec.c
runs a job:

    ctx->job_finished = false;
    ret = ctx->codec_ops->run(ctx);

    v4l2_m2m_buf_copy_metadata(src_buf, dst_buf);

completing the M2M job inside mjpeg_job_done() before aml_mjpeg_dec_run()
wakes up and stops the hardware creates several issues:

1. v4l2_m2m_buf_done_and_job_finish() returns src_buf and dst_buf to vb2 and
wakes userspace poll() / VIDIOC_DQBUF waiters before aml_mjpeg_dec_run()
returns to m2mops_vdec_device_run() to call
v4l2_m2m_buf_copy_metadata(src_buf, dst_buf). Userspace can dequeue dst_buf
without its copied timestamp, field, timecode, or flags, or re-queue/free
the buffers before v4l2_m2m_buf_copy_metadata() touches them.

2. v4l2_m2m_buf_done_and_job_finish() clears TRANS_RUNNING and schedules the
next M2M job via v4l2_m2m_schedule_next_job(). A subsequent job can enter
m2mops_vdec_device_run() on another CPU, set ctx->int_cond = 0, and call
aml_start_vdec_hw(hw) before the previous job's aml_mjpeg_dec_run() finishes
waking up and executes ctx->int_cond = 0 and aml_stop_vdec_hw(hw), stopping
and resetting the VDEC hardware mid-decode.

3. Because v4l2_m2m_cancel_job() only waits for TRANS_RUNNING to be cleared,
a concurrent VIDIOC_STREAMOFF or fops_vcodec_release() can proceed into
vb2ops_vdec_stop_streaming() -> aml_mjpeg_exit() (freeing m) and kfree(ctx)
while the job thread is still executing ctx->int_cond = 0,
aml_stop_vdec_hw(hw), and m2mops_vdec_device_run().

4. mjpeg_canvas_free() releases the DMC canvases and v4l2_m2m_buf_done()
invalidates CPU caches for the capture buffer before aml_stop_vdec_hw(hw)
stops the AMRISC processor and waits for LMEM_DMA_CTRL and WRRSP_LMEM writes
to drain.

Should aml_mjpeg_dec_run() stop the hardware after wait_event_timeout()
before the canvases are freed and the M2M job is finished?

[ ... ]

[Severity: Critical]
Can aml_mjpeg_exit() reset the shared VDEC core while another context is
decoding, or free m while mjpeg_threaded_isr() is still running?

First, ctx->dos_clk_en is set to 1 in m2mops_vdec_device_run() on the first
job and is never cleared. In aml_mjpeg_exit(), aml_stop_vdec_hw(hw) is
called whenever ctx->dos_clk_en is set, without checking if
hw->curr_ctx == m. If userspace calls VIDIOC_STREAMOFF or closes an idle
MJPEG context while another VDEC context is actively decoding on the shared
hardware, aml_stop_vdec_hw(hw) will halt the AMRISC processor and reset the
core mid-frame.

Second, aml_mjpeg_exit() clears hw->curr_ctx and immediately calls kfree(m)
without masking VDEC_ASSIST_MBOX1_MASK or calling
synchronize_irq(hw->dec_irq). If mjpeg_threaded_isr() is in flight and has
already read m = hw->curr_ctx (for example, right after a timeout or job
completion), can it dereference m and m->v4l2_ctx after kfree(m)?

[Severity: Medium]
What happens during format negotiation when userspace configures MJPEG on the
OUTPUT queue?

In drivers/media/platform/amlogic/vdec/aml_vdec.c:

1. vidioc_vdec_enum_fmt() enumerates all AML_FMT_FRAME formats (NV21M, NV21,
NV12M, YUV420, NV12) on the CAPTURE queue regardless of the selected OUTPUT
codec.

2. vdec_s_fmt_output() copies the OUTPUT pix_mp into ctx->pix_fmt[AML_FMT_DST]
and leaves ctx->dec_fmt[AML_FMT_DST] at its initial default of
V4L2_PIX_FMT_NV12 instead of updating the CAPTURE format to
V4L2_PIX_FMT_YUV420:

    ret = vdec_try_fmt_mp(ctx, f->type, pix_mp);
    set_pic_info(ctx, pix_mp, f->type);

    ctx->pix_fmt[AML_FMT_SRC] = *pix_mp;
    ctx->pix_fmt[AML_FMT_DST] = *pix_mp;

3. vdec_s_fmt_capture() accepts NV12, NV21, NV12M, or NV21M when the OUTPUT
format is JPEG (and accepts YUV420 when the OUTPUT format is H.264 or
MPEG-2), so buffer allocation and VIDIOC_STREAMON succeed and only fail
later here in aml_mjpeg_dec_run() with -EINVAL.

Should VIDIOC_ENUM_FMT, vdec_s_fmt_output(), and vdec_s_fmt_capture()
restrict and default the CAPTURE format to V4L2_PIX_FMT_YUV420 when the
OUTPUT codec is CODEC_TYPE_MJPEG?

[Severity: High]
Can vb2_plane_vaddr(&src->vb2_buf, 0) return NULL here and in
mjpeg_prepare_input()?

In aml_vdec_queue_init(), the OUTPUT queue is configured with
io_modes = VB2_MMAP | VB2_DMABUF and mem_ops = &vb2_dma_contig_memops. When
userspace queues a VB2_MEMORY_DMABUF buffer whose exporter does not implement
vmap or when dma_buf_vmap_unlocked() fails, vb2_plane_vaddr() returns NULL
(unlike h264_stage_with_headers() in h264.c, which checks for a NULL
virtual address).

When vb2_plane_vaddr() returns NULL and the payload length is at least 4,
mjpeg_supported() will dereference p[0]:

    if (len < 4 || p[0] != 0xff || p[1] != 0xd8)
        return false;

and mjpeg_prepare_input() also calls vb2_plane_vaddr(vb, 0) without a NULL
check before reading in[3].

[Severity: High]
If wait_event_timeout() expires just as the VDEC interrupt fires and
mjpeg_threaded_isr() runs on another CPU, can the timeout path race with
mjpeg_job_done()?

On timeout, aml_mjpeg_dec_run() does not mask VDEC_ASSIST_MBOX1_MASK, call
synchronize_irq(hw->dec_irq), clear hw->curr_ctx, or set
ctx->job_finished = true before stopping the hardware and freeing the
canvases.

As a result:

1. Both mjpeg_threaded_isr() -> mjpeg_job_done() and the timeout path call
mjpeg_canvas_free(hw, m) without locking:

    for (i = 0; i < ARRAY_SIZE(m->canvas); i++) {
        if (m->canvas[i] >= 0)
            meson_canvas_free(hw->canvas, m->canvas[i]);
        m->canvas[i] = -1;
    }

Because m->canvas[i] is only set to -1 after meson_canvas_free() returns,
both threads can free the same canvas indices concurrently.

2. When aml_mjpeg_dec_run() returns -ETIMEDOUT to m2mops_vdec_device_run() in
drivers/media/platform/amlogic/vdec/aml_vdec.c:

    if (ret < 0 && !ctx->job_finished)
        goto err_cancel_job;
    ...
err_cancel_job:
    if (src_req)
        v4l2_ctrl_request_complete(src_req, &ctx->ctrl_handler);
    v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx,
                                     VB2_BUF_STATE_ERROR);

m2mops_vdec_device_run() checks !ctx->job_finished without synchronization
against mjpeg_threaded_isr(), so both mjpeg_job_done() and err_cancel_job can
call v4l2_ctrl_request_complete() and v4l2_m2m_buf_done_and_job_finish() for
the same job.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192507.210058-1-christian.koever-draxl@student.uibk.ac.at?part=6