Re: [PATCH v5 09/14] media: mediatek: vcodec: add debug information

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Incorrect debug output string for V4L2_PIX_FMT_MT2110R (Typo)

Pre-existing issues:
- [Critical] Use-After-Free and List Corruption in debugfs due to Lock Mismatch
- [High] Out-of-Bounds Read in debugfs due to missing null terminator
--

commit 87b57cf0a11ac27db38b48d00f4d66f14eff0f96
Author: Yunfei Dong <yunfei.dong@mediatek.com>

media: mediatek: vcodec: add debug information
    
Print hevc/av1 output format and 10bit capture format
information to debug.

> 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..b3c69c46240cf 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_dbgfs.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_dbgfs.c

[ ... ]

> @@ -45,6 +53,14 @@ static void mtk_vdec_dbgfs_get_format_type(struct mtk_vcodec_dec_ctx *ctx, char
>  		curr_len = snprintf(buf + *used, total - *used,
>  				    "\tcapture format: MT21C\n");
>  		break;
> +	case V4L2_PIX_FMT_MT2110T:
> +		curr_len = snprintf(buf + *used, total - *used,
> +				    "\tcapture format: MT2110T (10bit tile mode)\n");
> +		break;
> +	case V4L2_PIX_FMT_MT2110R:
> +		curr_len = snprintf(buf + *used, total - *used,
> +				    "\tcapture format: MT2110T (10bit raster mode)\n");

[Severity: Low]
Does this incorrectly print MT2110T instead of MT2110R for the raster mode
format?

> +		break;
>  	default:
>  		curr_len = snprintf(buf + *used, total - *used,
>  				    "\tunsupported capture format: 0x%x\n",
> @@ -122,9 +138,10 @@ static ssize_t mtk_vdec_dbgfs_read(struct file *filp, char __user *ubuf,

[Severity: High]
This is a pre-existing issue, but does mtk_vdec_dbgfs_read() safely handle
dbgfs->dbgfs_buf?

Earlier in this function, the code checks:

if (strstr(dbgfs->dbgfs_buf, "-help") || dbgfs->buf_size == 1) {

However, mtk_vdec_dbgfs_write() uses simple_write_to_buffer() to copy user
payload into dbgfs->dbgfs_buf without ensuring the string is null-terminated.
Can this cause strstr() to read out-of-bounds?

[Severity: Critical]
This is a pre-existing issue, but is the list traversal in
mtk_vdec_dbgfs_read() fully protected against concurrent modification?

The traversal iterates over &dbgfs->dbgfs_head while holding
dbgfs->dbgfs_lock:

mtk_vcodec_dbgfs.c:mtk_vdec_dbgfs_read() {
    ...
    mutex_lock(&dbgfs->dbgfs_lock);
    list_for_each_entry(dbgfs_inst, &dbgfs->dbgfs_head, node) {
    ...
}

However, mtk_vcodec_dbgfs_remove() mutates this same list and frees the
nodes without acquiring dbgfs->dbgfs_lock:

mtk_vcodec_dbgfs.c:mtk_vcodec_dbgfs_remove() {
    ...
    list_for_each_entry(dbgfs_inst, &vcodec_dev->dbgfs.dbgfs_head, node) {
        if (dbgfs_inst->inst_id == ctx_id) {
            ...
            list_del(&dbgfs_inst->node);
            kfree(dbgfs_inst);
    ...
}

Can this lead to a use-after-free or list corruption if
mtk_vcodec_dbgfs_remove() races with the mtk_vdec_dbgfs_read() traversal?

>  
>  		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;
>  		}
>

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