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

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 <[email protected]>

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/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic