Re: [PATCH v2] media: meson: vdec: fix use-after-free of in-use frames 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] 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