Re: [PATCH] media: mtk-vcodec: vdec_vp8_if: guard list_first_entry on empty fb node pool

[Nicolas Dufresne]

Le vendredi 23 octobre 2026 à 15:56 +0800, Li Ximo a écrit :
> From: Wang Haoran <haoranwangsec@xxxxxxxxx>

Concistency comment, use "media: mediatek: vcodec:" subject prefix.

> 
> list_first_entry() does not return NULL on an empty list; it returns a
> wild pointer computed by container_of() on the list head, and
> dereferencing it corrupts memory. vdec_vp8_if.c has three unguarded
> list_first_entry() calls on inst->available_fb_node_list: two in
> vp8_dec_finish() (one unconditional, one on the show_frame path) and
> one in add_fb_to_free_list(). The pool holds a fixed number of nodes
> (VP8_MAX_FRM_BUF_NODE_NUM = 10) prefilled by init_list(), nodes leave
> the pool on every successful decode (two per show_frame decode and via
> the vpu error / resolution-change paths), and they are returned to the
> pool only through the userspace-driven get_disp_fb()/get_free_fb()
> calls -- which themselves use list_first_entry_or_null(), i.e. the
> driver already treats the pool as exhaustible on the return path but
> not on the take paths. If the VPU firmware keeps reporting a stale or
> zero prev_y_dma, or if userspace decodes more frames than it
> reclaims, the pool is exhausted and the next list_first_entry()
> returns a wild pointer into struct vdec_vp8_inst, after which
> node->fb = ... and list_move_tail() perform an in-structure
> out-of-bounds write and list corruption. This was found by code
> audit; we do not have a reproducer on real hardware.
> 
> Fix this by switching the three take sites to
> list_first_entry_or_null() and bailing out when the pool is empty,
> matching the existing style of get_disp_fb()/get_free_fb() in the same
> file. On exhaustion vp8_dec_finish() now logs an error and stops
> tracking the frame instead of corrupting memory, and
> add_fb_to_free_list() simply drops the buffer.

Can you reduce this in your next version. Please start with stating what you
fix, such as "Fix possible null derefence", drop the "possible" if you can
actually reproduce. And then explain if needed. This iteration is just too much
and to hard to parse (in comparison to the actual code change).

> 
> Reported-by: Wang Haoran <haoranwangsec@xxxxxxxxx>
> Reported-by: Li Ximo <lixicaicai@xxxxxxxxx>
> Signed-off-by: Wang Haoran <haoranwangsec@xxxxxxxxx>
> ---
>  .../vcodec/decoder/vdec/vdec_vp8_if.c         | 22 ++++++++++++++-----
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp8_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp8_if.c
> index fd15d462b..ba34c71f7 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp8_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp8_if.c
> @@ -308,15 +308,23 @@ static void vp8_dec_finish(struct vdec_vp8_inst *inst)
>  	}
>  
>  	/* available_fb_node_list -> fb_use_list */
> -	node = list_first_entry(&inst->available_fb_node_list,
> -				struct vdec_fb_node, list);
> +	node = list_first_entry_or_null(&inst->available_fb_node_list,
> +					struct vdec_fb_node, list);
> +	if (!node) {
> +		mtk_vdec_err(inst->ctx, "no available fb node");
> +		return;
> +	}
>  	node->fb = inst->cur_fb;
>  	list_move_tail(&node->list, &inst->fb_use_list);
>  
>  	/* available_fb_node_list -> fb_disp_list */
>  	if (inst->vsi->dec.show_frame) {
> -		node = list_first_entry(&inst->available_fb_node_list,
> -					struct vdec_fb_node, list);
> +		node = list_first_entry_or_null(&inst->available_fb_node_list,
> +						struct vdec_fb_node, list);
> +		if (!node) {
> +			mtk_vdec_err(inst->ctx, "no available fb node");
> +			return;
> +		}
>  		node->fb = inst->cur_fb;
>  		list_move_tail(&node->list, &inst->fb_disp_list);
>  	}
> @@ -352,8 +360,10 @@ static void add_fb_to_free_list(struct vdec_vp8_inst *inst, void *fb)
>  	struct vdec_fb_node *node;
>  
>  	if (fb) {
> -		node = list_first_entry(&inst->available_fb_node_list,
> -					struct vdec_fb_node, list);
> +		node = list_first_entry_or_null(&inst->available_fb_node_list,
> +						struct vdec_fb_node, list);
> +		if (!node)
> +			return;
>  		node->fb = fb;
>  		list_move_tail(&node->list, &inst->fb_free_list);
>  	}

Attachment: signature.asc
Description: This is a digitally signed message part