Re: [PATCH] media: meson: vdec: fix use-after-free of prev_frame in codec_vp9_rm_noshow_frame()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Flushing the decoder leaves a dangling pointer in `vp9->prev_frame`, leading to a Use-After-Free.
- [High] Deleting active reference frames causes hardware desync when configuring reference canvas data.
- [High] Kernel NULL pointer dereference in `codec_vp9_set_mpred_mv()` if a stream begins with a non-key frame or after a resolution change.
- [High] An infinite loop in `codec_vp9_get_new_frame()` causes a hard kernel lockup if the ready queue only contains buffers currently used as reference frames.
--

commit 6e91e5b5beb7355817230e0312560c02d6e0441c
Author: Doruk Tan Ozturk <doruk@0sec.ai>
Subject: media: meson: vdec: fix use-after-free of prev_frame in codec_vp9_rm_noshow_frame()

This commit adds a check in codec_vp9_rm_noshow_frame() to avoid freeing
vp9->prev_frame. It prevents a use-after-free regression that occurs when the
current frame is a non-show inter frame and the previous decoded frame was an
alt-ref frame.

> diff --git a/drivers/staging/media/meson/vdec/codec_vp9.c b/drivers/staging/media/meson/vdec/codec_vp9.c
> --- a/drivers/staging/media/meson/vdec/codec_vp9.c
> +++ b/drivers/staging/media/meson/vdec/codec_vp9.c
> @@ -1247,6 +1247,15 @@ static void codec_vp9_rm_noshow_frame(struct amvdec_session *sess)
>  		if (tmp->show)
>  			continue;

[Severity: High]
This is a pre-existing issue, but does this loop delete active reference frames
without consulting codec_vp9_is_ref()?

If a stream contains multiple non-show frames and one is an active reference,
it appears it is unconditionally deleted here. Later, codec_vp9_sync_ref()
could fail to find it, leaving vp9->frame_refs array with missing entries.

This would cause codec_vp9_set_refs() to skip writing to the auto-incrementing
HEVCD_MPP_ANC_CANVAS_DATA_ADDR hardware port:

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_set_refs() {
    ...
    if (!frame)
        continue;
    ...
    amvdec_write_dos(core, HEVCD_MPP_ANC_CANVAS_DATA_ADDR,
    ...
}

Could this missing write desync the hardware configuration sequence and
corrupt the decoding?

> +		/*
> +		 * prev_frame is still referenced by the MV predictor in
> +		 * codec_vp9_set_mpred_mv(); the sibling codec_vp9_show_frame()
> +		 * already excludes it before freeing. Do the same here to avoid
> +		 * a use-after-free of vp9->prev_frame.
> +		 */
> +		if (tmp == vp9->prev_frame)
> +			continue;
> +
>  		pr_debug("rm noshow: %u\n", tmp->index);
>  		v4l2_m2m_buf_queue(sess->m2m_ctx, tmp->vbuf);
>  		list_del(&tmp->list);


[Severity: High]
This is a pre-existing issue, but is there a risk of a null pointer
dereference in codec_vp9_set_mpred_mv() if the stream begins with a
non-key frame?

In codec_vp9_process_frame(), codec_vp9_set_mpred_mv() is called based on
the frame type:

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_process_frame() {
    ...
    if (param->p.frame_type != KEY_FRAME && !intra_only) {
        codec_vp9_set_mc(sess, vp9);
        codec_vp9_set_mpred_mv(core, vp9);
    }
    ...
}

If vp9->prev_frame is null because a keyframe was missing, wouldn't the
unconditional dereference in codec_vp9_set_mpred_mv() cause a crash?

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_set_mpred_mv() {
    ...
    int use_prev_frame_mvs = vp9->prev_frame->width ==
                             vp9->cur_frame->width &&
                             vp9->prev_frame->height ==
    ...
}


[Severity: High]
This is a pre-existing issue, but could codec_vp9_get_new_frame() get stuck
in an infinite loop if the ready queue only contains buffers currently used
as reference frames?

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_get_new_frame() {
    ...
    while (codec_vp9_get_frame_by_idx(vp9, vbuf->vb2_buf.index)) {
        struct vb2_v4l2_buffer *old_vbuf = vbuf;

        vbuf = v4l2_m2m_dst_buf_remove(sess->m2m_ctx);
        v4l2_m2m_buf_queue(sess->m2m_ctx, old_vbuf);
    ...
}

If userspace immediately queues back buffers that are still held by the driver
as active reference frames, and no other buffers are in the queue, does this
continually pop and push the same referenced buffers forever?


[Severity: Critical]
This is a pre-existing issue, but does flushing the decoder leave a
dangling pointer in vp9->prev_frame?

When flushing, codec_vp9_flush_output() frees the reference frames list:

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_flush_output() {
    ...
    list_for_each_entry_safe(tmp, n, &vp9->ref_frames_list, list) {
        ...
        list_del(&tmp->list);
        kfree(tmp);
    }
    ...
}

Since vp9->prev_frame is not cleared here, if decoding is subsequently
resumed with an INTER_FRAME, codec_vp9_resume() would process it and call
codec_vp9_set_mpred_mv(), which dereferences vp9->prev_frame:

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_set_mpred_mv() {
    ...
    int use_prev_frame_mvs = vp9->prev_frame->width ==
                             vp9->cur_frame->width &&
    ...
}

Could this sequence lead to accessing freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626164025.52694-1-doruk@0sec.ai?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic