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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: mediatek: vcodec: UAF due to lock mismatch in debugfs iteration
--

commit 3dd0220ecce9f2ca273e1906b8312cd6795ec662
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: vcodec: add debug information

This commit adds debug information for HEVC/AV1 output formats and 10-bit
capture formats. It updates the debugfs read function to include 10-bit
bitstream information.

> 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,
>  
>  		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);

[Severity: High]
Can a concurrent release of the video device cause a use-after-free when 
accessing ctx in this debugfs read path?

In mtk_vdec_dbgfs_read(), the iteration over dbgfs_head is protected by 
dbgfs->dbgfs_lock. However, a concurrent device release calling 
mtk_vcodec_dbgfs_remove() removes and frees dbgfs_inst without acquiring 
dbgfs_lock:

drivers/media/platform/mediatek/vcodec/common/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);
            return;
        }
    }
}

Because mtk_vcodec_dbgfs_remove() does not take dbgfs_lock, 
mtk_vdec_dbgfs_read() can dereference a freed dbgfs_inst and a freed ctx 
when a user reads the debugfs file at the same time the device is closed.

Should the list removal in mtk_vcodec_dbgfs_remove() be protected by 
dbgfs_lock?

>  			used_len += curr_len;
>  		}
>

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