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

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

[Doruk Tan Ozturk]

codec_vp9_rm_noshow_frame() frees the first non-shown reference frame on
ref_frames_list without excluding frames that are still in use. When the
previously decoded frame was a non-show (alt-ref) frame and the current
frame is a non-show inter frame, the freed object is the one
vp9->prev_frame still points to; codec_vp9_set_mpred_mv() then
dereferences the stale pointer (use_prev_frame_mvs and
codec_vp9_get_frame_mv_paddr()), a use-after-free. Freeing a frame that
is still an active reference (codec_vp9_is_ref()) or the current frame
has the same in-use-then-free shape and additionally desyncs the
reference bookkeeping in codec_vp9_sync_ref().

The sibling cleanup codec_vp9_show_frame() already guards exactly these
cases before freeing:

	if (codec_vp9_is_ref(vp9, tmp) || tmp == vp9->prev_frame)
		continue;

rm_noshow_frame() simply omits the same check. Add it, also skipping
cur_frame, so both cleanup paths agree on which frames are safe to free.

The fields that drive this path (show_frame, frame_type, intra_only) are
parsed from the VP9 bitstream, so a crafted stream fed to the stateless
decoder can trigger the free-then-use.

Found by 0sec's autonomous vulnerability analysis (https://0sec.ai).
Found by static analysis; not yet runtime-reproduced (Amlogic Meson
hardware required).

Fixes: 00c43088aa68 ("media: meson: vdec: add VP9 decoder support")
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
v2: Per Dan Carpenter's review, also skip active reference frames
    (codec_vp9_is_ref()) and cur_frame, matching codec_vp9_show_frame()
    exactly — freeing an in-use altref/reference frame here also caused a
    codec_vp9_sync_ref() desync, not just the prev_frame UAF.

 drivers/staging/media/meson/vdec/codec_vp9.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/codec_vp9.c b/drivers/staging/media/meson/vdec/codec_vp9.c
index 8e80ecf84193..dad75950933c 100644
--- a/drivers/staging/media/meson/vdec/codec_vp9.c
+++ b/drivers/staging/media/meson/vdec/codec_vp9.c
@@ -1238,6 +1238,8 @@ static void codec_vp9_show_existing_frame(struct codec_vp9 *vp9)
 	pr_debug("showing frame %u\n", param->p.frame_to_show_idx);
 }
 
+static bool codec_vp9_is_ref(struct codec_vp9 *vp9, struct vp9_frame *frame);
+
 static void codec_vp9_rm_noshow_frame(struct amvdec_session *sess)
 {
 	struct codec_vp9 *vp9 = sess->priv;
@@ -1247,6 +1249,18 @@ static void codec_vp9_rm_noshow_frame(struct amvdec_session *sess)
 		if (tmp->show)
 			continue;
 
+		/*
+		 * Mirror codec_vp9_show_frame(): never free an active
+		 * reference frame, the previously decoded frame, or the
+		 * current frame here. prev_frame is still dereferenced by the
+		 * MV predictor in codec_vp9_set_mpred_mv(), and freeing an
+		 * in-use altref/reference also desyncs codec_vp9_sync_ref();
+		 * either is a use-after-free of an in-use frame.
+		 */
+		if (codec_vp9_is_ref(vp9, tmp) || tmp == vp9->prev_frame ||
+		    tmp == vp9->cur_frame)
+			continue;
+
 		pr_debug("rm noshow: %u\n", tmp->index);
 		v4l2_m2m_buf_queue(sess->m2m_ctx, tmp->vbuf);
 		list_del(&tmp->list);
-- 
2.53.0


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

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Dangling `vp9->prev_frame` pointer after a decoder flush leads to a Use-After-Free and potential out-of-bounds DMA.
- [Critical] Unbounded `width` and `height` parsed from the bitstream lead to out-of-bounds hardware DMA reads/writes.
- [High] Missing NULL check for `vp9->prev_frame` in `codec_vp9_set_mpred_mv()` causes a kernel panic if the first frame is a non-keyframe.
- [High] Infinite loop in `codec_vp9_get_new_frame()` when all queued V4L2 destination buffers are active references.
--

commit 1bdbcdca407cee95e0f170623c256ff600931474
Author: Doruk Tan Ozturk <doruk@0sec.ai>

    media: meson: vdec: fix use-after-free of in-use frames in codec_vp9_rm_noshow_frame()
    
    This patch adds checks to codec_vp9_rm_noshow_frame() to prevent freeing
    active reference frames, the current frame, and the previously decoded
    frame. It aims to fix a use-after-free bug that could be triggered by a
    malformed VP9 bitstream.

> diff --git a/drivers/staging/media/meson/vdec/codec_vp9.c b/drivers/staging/media/meson/vdec/codec_vp9.c
> index 8e80ecf84193d..dad75950933cb 100644
> --- a/drivers/staging/media/meson/vdec/codec_vp9.c
> +++ b/drivers/staging/media/meson/vdec/codec_vp9.c

[ ... ]

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

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 &&
    ...
}

[Severity: High]
This is a pre-existing issue, but is it possible for a kernel panic to occur
here? 

A newly initialized VP9 decoder session has vp9->cur_frame = NULL and
vp9->prev_frame = NULL. If the very first frame processed by the hardware is
parsed as a non-keyframe (INTER_FRAME), codec_vp9_process_frame()
unconditionally calls codec_vp9_set_mpred_mv().

Inside codec_vp9_set_mpred_mv(), vp9->prev_frame->width and other fields are
dereferenced without a NULL check.

Since this seems reachable by an unprivileged user feeding a malformed
bitstream (starting with an INTER_FRAME) to the V4L2 device, could this lead
to a NULL pointer dereference?

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

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_set_mpred_mv() {
    ...
	mpred_mv_rd_end_addr =
			codec_vp9_get_frame_mv_paddr(vp9, vp9->prev_frame) +
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this sequence leave a dangling pointer?

When the V4L2 decoder receives a flush/stop command, the .drain callback
(codec_vp9_flush_output()) frees all frames in vp9->ref_frames_list.
However, it appears to leave vp9->prev_frame pointing to these freed
allocations.

If the decoder is subsequently resumed, processing an INTER_FRAME will call
codec_vp9_set_mpred_mv(), which dereferences the dangling vp9->prev_frame.
Furthermore, it passes the freed pointer to codec_vp9_get_frame_mv_paddr(),
which reads the index field from the freed memory to calculate the hardware
DMA read/write addresses.

Could an attacker use a STOP command followed by a RESUME/START command with
an INTER_FRAME to trigger a use-after-free and potential out-of-bounds DMA
access?

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);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but could this while loop run
infinitely?

In codec_vp9_get_new_frame(), a while loop pulls a destination buffer from
the V4L2 queue and checks if it is currently an active VP9 reference using
codec_vp9_get_frame_by_idx(). If it is, the loop pushes it to the back of
the queue.

If a user maliciously or inadvertently queues only buffers that are currently
held as active references by the decoder, the loop will continually dequeue
and requeue the exact same set of buffers. Since v4l2_m2m_dst_buf_remove()
never returns NULL when num_rdy > 0, could this cause a hard lockup?

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_process_rpm() {
    ...
	vp9->lcu_total = pic_width_lcu * pic_height_lcu;
    ...
}

drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_set_mpred_mv() {
    ...
	mpred_mv_rd_end_addr =
			codec_vp9_get_frame_mv_paddr(vp9, vp9->prev_frame) +
			(vp9->lcu_total * MV_MEM_UNIT);
	amvdec_write_dos(core, HEVC_MPRED_MV_RD_END_ADDR, mpred_mv_rd_end_addr);
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this code properly bound the width
and height parsed from the bitstream?

In codec_vp9_process_rpm(), vp9->width and vp9->height are taken directly
from the firmware's RPM buffer without bounds checking. These dictate
vp9->lcu_total. 

A malicious VP9 stream could provide dimensions up to 65535x65535. When
decoding resumes, codec_vp9_set_mpred_mv() programs the hardware DMA register
HEVC_MPRED_MV_RD_END_ADDR using vp9->lcu_total. 

Since the underlying workspace size is fixed, could the hardware be commanded
to perform DMA reads/writes beyond the allocated workspace?

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

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