[PATCH 0/2] media: meson: vdec: fix two more VP9 reference-frame lifetime bugs

[PATCH 0/2] media: meson: vdec: fix two more VP9 reference-frame lifetime bugs

[Doruk Tan Ozturk]

While reviewing the earlier VP9 prev_frame use-after-free fix, a
Sashiko AI review of that change surfaced two further reference-frame
lifetime bugs in the same decoder, both rooted in vp9->prev_frame /
vp9->cur_frame not being managed across all decode entry points.

Patch 1 clears the cached prev_frame/cur_frame pointers in the .drain
flush path, which frees every ref_frames_list node but left those two
pointers aliasing freed memory; a decode resuming with an inter frame
would then dereference freed vp9_frame storage in
codec_vp9_set_mpred_mv() (use-after-free).

Patch 2 guards codec_vp9_set_mpred_mv() against vp9->prev_frame being
NULL, which happens when the first decoded frame is an inter frame
(malformed/adversarial input, or the first frame after a flush). The
function dereferences prev_frame unconditionally, both for the
use_prev_frame_mvs computation and for the previous-frame MV read
register programming, so the NULL case is a NULL pointer dereference.

Both issues were found by static analysis and are not yet runtime-
reproduced (Amlogic Meson hardware required).

Found by 0sec's autonomous vulnerability analysis (https://0sec.ai).

Doruk Tan Ozturk (2):
  media: meson: vdec: clear stale prev_frame/cur_frame on flush
  media: meson: vdec: guard against NULL prev_frame in
    codec_vp9_set_mpred_mv()

 drivers/staging/media/meson/vdec/codec_vp9.c | 37 ++++++++++++++++----
 1 file changed, 30 insertions(+), 7 deletions(-)

-- 
2.53.0


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

[PATCH 1/2] media: meson: vdec: clear stale prev_frame/cur_frame on flush

[Doruk Tan Ozturk]

codec_vp9_flush_output() (the .drain callback) walks ref_frames_list
and kfree()s every vp9_frame node, but never clears vp9->prev_frame
or vp9->cur_frame, which alias nodes that were just freed.

If decoding resumes after a flush with an inter (non-key) frame,
codec_vp9_process_frame() calls codec_vp9_set_mpred_mv(), which
dereferences vp9->prev_frame->{width,height,intra_only,show,type}
and feeds vp9->prev_frame to codec_vp9_get_frame_mv_paddr(). With
prev_frame still pointing at freed memory this is a use-after-free.

Clear both cached pointers once the list has been freed so a resumed
decode starts from a clean state.

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>
---
 drivers/staging/media/meson/vdec/codec_vp9.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/codec_vp9.c b/drivers/staging/media/meson/vdec/codec_vp9.c
index 8e80ecf84193..5ca27930239f 100644
--- a/drivers/staging/media/meson/vdec/codec_vp9.c
+++ b/drivers/staging/media/meson/vdec/codec_vp9.c
@@ -681,6 +681,16 @@ static void codec_vp9_flush_output(struct amvdec_session *sess)
 		list_del(&tmp->list);
 		kfree(tmp);
 	}
+
+	/*
+	 * All ref_frames_list nodes have been freed above. Drop the cached
+	 * pointers so a decode resuming after the flush (e.g. an inter frame
+	 * following a drain) cannot dereference freed vp9_frame memory in
+	 * codec_vp9_set_mpred_mv().
+	 */
+	vp9->prev_frame = NULL;
+	vp9->cur_frame = NULL;
+
 	mutex_unlock(&vp9->lock);
 }
 
-- 
2.53.0


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

[PATCH 2/2] media: meson: vdec: guard against NULL prev_frame in codec_vp9_set_mpred_mv()

[Doruk Tan Ozturk]

codec_vp9_set_mpred_mv() dereferences vp9->prev_frame unconditionally,
both when computing use_prev_frame_mvs (prev_frame->width, ->height,
->intra_only, ->show, ->type) and when programming the previous-frame
MV read registers via codec_vp9_get_frame_mv_paddr(vp9, vp9->prev_frame)
(HEVC_MPRED_MV_RD_START_ADDR, HEVC_MPRED_MV_RPTR and the RD_END_ADDR
computation).

vp9->prev_frame is only assigned (= vp9->cur_frame) after a frame has
been processed, and is NULL after allocation and after a flush. The
caller, codec_vp9_process_frame(), reaches codec_vp9_set_mpred_mv()
whenever the frame is a non-key, non-intra-only frame, without checking
that a previous frame exists. A stream whose first decoded frame is an
inter frame (malformed/adversarial input, or the first frame after a
drain) therefore triggers a NULL pointer dereference.

Disable previous-frame MV use (clear HEVC_MPRED_CTRL4 BIT(6), which the
function already does up front) and return early when prev_frame is
NULL, before any dereference. There are no previous-frame motion
vectors to consume in that case, so this is the correct behaviour as
well as the safe one.

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>
---
 drivers/staging/media/meson/vdec/codec_vp9.c | 27 +++++++++++++++-----
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/meson/vdec/codec_vp9.c b/drivers/staging/media/meson/vdec/codec_vp9.c
index 5ca27930239f..1df641202687 100644
--- a/drivers/staging/media/meson/vdec/codec_vp9.c
+++ b/drivers/staging/media/meson/vdec/codec_vp9.c
@@ -993,19 +993,32 @@ static void codec_vp9_set_mpred_mv(struct amvdec_core *core,
 				   struct codec_vp9 *vp9)
 {
 	int mpred_mv_rd_end_addr;
-	int use_prev_frame_mvs = vp9->prev_frame->width ==
-					vp9->cur_frame->width &&
-				 vp9->prev_frame->height ==
-					vp9->cur_frame->height &&
-				 !vp9->prev_frame->intra_only &&
-				 vp9->prev_frame->show &&
-				 vp9->prev_frame->type != KEY_FRAME;
+	int use_prev_frame_mvs;
 
 	amvdec_write_dos(core, HEVC_MPRED_CTRL3, 0x24122412);
 	amvdec_write_dos(core, HEVC_MPRED_ABV_START_ADDR,
 			 vp9->workspace_paddr + MPRED_ABV_OFFSET);
 
 	amvdec_clear_dos_bits(core, HEVC_MPRED_CTRL4, BIT(6));
+
+	/*
+	 * prev_frame is NULL when an inter frame is the first frame decoded
+	 * (e.g. a stream starting on a non-key frame, or the first frame
+	 * after a flush). There are no previous-frame motion vectors to use
+	 * and every read below would dereference a NULL pointer, so leave
+	 * prev-MV use disabled (BIT(6) already cleared) and bail out.
+	 */
+	if (!vp9->prev_frame)
+		return;
+
+	use_prev_frame_mvs = vp9->prev_frame->width ==
+				vp9->cur_frame->width &&
+			     vp9->prev_frame->height ==
+				vp9->cur_frame->height &&
+			     !vp9->prev_frame->intra_only &&
+			     vp9->prev_frame->show &&
+			     vp9->prev_frame->type != KEY_FRAME;
+
 	if (use_prev_frame_mvs)
 		amvdec_write_dos_bits(core, HEVC_MPRED_CTRL4, BIT(6));
 
-- 
2.53.0


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

Re: [PATCH 2/2] media: meson: vdec: guard against NULL prev_frame in codec_vp9_set_mpred_mv()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The patch introduces a critical vulnerability by using an early return to fix a NULL pointer dereference. When `vp9->prev_frame` is NULL, `codec_vp9_set_mpred_mv()` returns early and skips programming the hardware's motion vector write registers (`HEVC_MPRED_MV_WR_START_ADDR` and `HEVC_MPRED_MV_WPTR`) for the current frame. When the decoder is subsequently started, it performs a wild DMA write to whatever uninitialized or stale physical address is lingering in the hardware register, causing arbitrary physical memory corruption.
--

commit bf11770a8f9c71282f324df9d5ac30ccdcb86996
Author: Doruk Tan Ozturk <doruk@0sec.ai>
media: meson: vdec: guard against NULL prev_frame in codec_vp9_set_mpred_mv()

This commit addresses a NULL pointer dereference in codec_vp9_set_mpred_mv()
by adding an early return when vp9->prev_frame is NULL. The crash would
previously trigger when decoding malformed bitstreams that start on a non-key
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
[ ... ]
> @@ -993,19 +993,32 @@ static void codec_vp9_set_mpred_mv(struct amvdec_core *core,
[ ... ]
> +	/*
> +	 * prev_frame is NULL when an inter frame is the first frame decoded
> +	 * (e.g. a stream starting on a non-key frame, or the first frame
> +	 * after a flush). There are no previous-frame motion vectors to use
> +	 * and every read below would dereference a NULL pointer, so leave
> +	 * prev-MV use disabled (BIT(6) already cleared) and bail out.
> +	 */
> +	if (!vp9->prev_frame)
> +		return;

[Severity: Critical]
Does this early return bypass the initialization of the hardware's motion
vector write registers for the current frame?

Looking at the rest of codec_vp9_set_mpred_mv(), the following registers are
set after this early return block:

	amvdec_write_dos(core, HEVC_MPRED_MV_WR_START_ADDR,
			 codec_vp9_get_frame_mv_paddr(vp9, vp9->cur_frame));
	amvdec_write_dos(core, HEVC_MPRED_MV_WPTR,
			 codec_vp9_get_frame_mv_paddr(vp9, vp9->cur_frame));

If these writes are skipped when vp9->prev_frame is NULL, could the decoder
perform unconstrained DMA writes to whatever uninitialized or stale physical
addresses are lingering in the hardware register when decoding begins?

Since this state can be reached via malformed streams from unprivileged users
as noted in the commit message, can this lead to arbitrary physical memory
corruption?

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

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

Re: [PATCH 1/2] media: meson: vdec: clear stale prev_frame/cur_frame on flush

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Use-After-Free of `vp9->prev_frame` when processing consecutive no-show frames.
- [High] Use-After-Free of `sess` due to missing asynchronous workqueue cancellation during device close.
--

commit 961faf66a025a06408c51eb9a5e24e4a4b8c4fc8
Author: Doruk Tan Ozturk <doruk@0sec.ai>

media: meson: vdec: clear stale prev_frame/cur_frame on flush

This commit clears stale prev_frame and cur_frame pointers in
codec_vp9_flush_output() to prevent a use-after-free when decoding resumes
after a flush. It ensures a clean state by dropping the cached pointers once
the list has been freed.

> diff --git a/drivers/staging/media/meson/vdec/codec_vp9.c b/drivers/staging/media/meson/vdec/codec_vp9.c
> index 8e80ecf84193d..5ca27930239f8 100644
> --- a/drivers/staging/media/meson/vdec/codec_vp9.c
> +++ b/drivers/staging/media/meson/vdec/codec_vp9.c
> @@ -681,6 +681,16 @@ static void codec_vp9_flush_output(struct amvdec_session *sess)
>  		list_del(&tmp->list);
>  		kfree(tmp);
>  	}
> +
> +	/*
> +	 * All ref_frames_list nodes have been freed above. Drop the cached
> +	 * pointers so a decode resuming after the flush (e.g. an inter frame
> +	 * following a drain) cannot dereference freed vp9_frame memory in
> +	 * codec_vp9_set_mpred_mv().
> +	 */
> +	vp9->prev_frame = NULL;
> +	vp9->cur_frame = NULL;
> +
>  	mutex_unlock(&vp9->lock);
>  }

[Severity: Critical]
This is a pre-existing issue, but I noticed another potential regression
involving vp9->prev_frame in codec_vp9_rm_noshow_frame().

When processing consecutive no-show frames, the first no-show frame is
unconditionally freed:

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

Does this leave vp9->prev_frame dangling if it aliased the freed frame?

Later on, when the decoder processes the current frame as an inter-frame,
the pointer seems to be dereferenced here:

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

Could a malformed stream with consecutive alt-ref frames trigger this path?

[Severity: High]
This is a pre-existing issue, but while reviewing the teardown paths for
this driver, I noticed a potential regression in vdec_close().

The session object is freed during close without canceling the workqueue:

drivers/staging/media/meson/vdec/vdec.c:vdec_close() {
    ...
	mutex_destroy(&sess->bufs_recycle_lock);

	kfree(sess);

	return 0;
}

Is it possible for the esparser_queue_work asynchronous workqueue to still
be active when this happens? If the workqueue executes concurrently, it
appears to access the newly freed session object:

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
	struct amvdec_session *sess =
		container_of(work, struct amvdec_session, esparser_queue_work);

	mutex_lock(&sess->lock);
    ...
}

Would calling cancel_work_sync() before freeing the session prevent this?

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

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

Re: [PATCH 0/2] media: meson: vdec: fix two more VP9 reference-frame lifetime bugs

[Doruk Tan Ozturk]

Please drop this series; both patches are wrong.

1/2 is mis-attributed: codec_vp9_flush_output() is the .drain handler and is
only followed by teardown (codec_vp9_stop), never a resume, so the "dangling
pointer on resume" path doesn't exist. The real stale-prev_frame deref is on
the source-change resume path, not flush.

2/2 fixes a real NULL deref but in the wrong place: the early return also skips
the current-frame MV-write register setup (HEVC_MPRED_MV_WR_START_ADDR /
HEVC_MPRED_MV_WPTR), leaving the hardware to DMA-write to a stale address.
Sashiko's review is correct. The right fix guards only the prev_frame reads
while keeping the cur_frame writes; I'll send that separately once tested.

The rm_noshow_frame() use-after-free ([PATCH v2] on the list) is independent
and unaffected.

Doruk

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