Re: [PATCH v6 06/14] media: mediatek: vcodec: add debug information

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Lack of adequate locking on the `dbgfs_head` instance list allows concurrent list modifications, leading to Use-After-Free and list corruption.
- [High] Misuse of `snprintf` return values combined with a TOCTOU race on `inst_count` leads to an out-of-bounds read and WARN_ON_ONCE.
--

--- Patch [6]: [PATCH v6 06/14] media: mediatek: vcodec: add debug information ---
commit 339b2440735f521e0732ed61e123f2dc8ea944d3
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: vcodec: add debug information

This commit adds hevc and av1 output format printing to the debugfs
output. It also introduces 10-bit capture format and bitstream
information to the debug output.

> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_dbgfs.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_dbgfs.c
> index 2da11521fc7b9..a9e52a32079d3 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_dbgfs.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_dbgfs.c

[ ... ]

> @@ -122,9 +138,10 @@ static ssize_t mtk_vdec_dbgfs_read(struct file *filp, char __user *ubuf,

[Severity: Critical]
This is a pre-existing issue, but does the iteration surrounding this code
adequately protect against concurrent list modifications?

In mtk_vdec_dbgfs_read(), the list is traversed under dbgfs_lock:

mutex_lock(&dbgfs->dbgfs_lock);
list_for_each_entry(dbgfs_inst, &dbgfs->dbgfs_head, node) {

However, if a user process rapidly opens and closes the V4L2 device,
mtk_vcodec_dbgfs_remove() unlinks and frees the list element locklessly with
respect to the debugfs read lock:

mtk_vcodec_dbgfs_remove()
    list_del(&dbgfs_inst->node);
    kfree(dbgfs_inst);

Since the two paths acquire different locks, they can execute concurrently.
Can this lead to a use-after-free or list corruption when the reader hits a
freed node or poisoned list pointers?

>  
>  		if (dbgfs_index[MTK_VDEC_DBGFS_PICINFO]) {
>  			curr_len = snprintf(buf + used_len, total_len - used_len,
> -					    "\treal(%dx%d)=>align(%dx%d)\n",
> +					    "\treal(%dx%d)=>align(%dx%d) 10bit(%d)\n",
>  					    ctx->picinfo.pic_w, ctx->picinfo.pic_h,
> -					    ctx->picinfo.buf_w, ctx->picinfo.buf_h);
> +					    ctx->picinfo.buf_w, ctx->picinfo.buf_h,
> +					    ctx->is_10bit_bitstream);
>  			used_len += curr_len;

[Severity: High]
This is a pre-existing issue, but could this accumulation of used_len result
in an out-of-bounds read and a kernel warning?

The total_len is calculated earlier based on a lockless read of inst_count:

mtk_vdec_dbgfs_read()
    int total_len = 200 * (dbgfs->inst_count == 0 ? 1 : dbgfs->inst_count);

If concurrent device opens increase the number of instances after this buffer
is allocated, used_len can exceed total_len.

When total_len - used_len becomes negative, snprintf receives a massive size
parameter. While vsnprintf catches sizes greater than INT_MAX and prevents a
heap overflow, it issues a WARN_ON_ONCE.

Furthermore, snprintf returns the length it would have written, which is
added to used_len. Later, simple_read_from_buffer() will use this inflated
used_len to read past the end of the heap allocation into userspace.

>  		}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720012056.1026551-1-kyrie.wu@mediatek.com?part=6