From: sashiko-bot@kernel.org
To: "Kyrie Wu" <kyrie.wu@mediatek.com>
Cc: conor+dt@kernel.org, media-ci@linuxtv.org, robh@kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH v7 05/14] media: mediatek: vcodec: get share memory address
Date: Wed, 23 Sep 2026 02:58:42 +0000 [thread overview]
Message-ID: <20260923025843.0398C1F00893@smtp.kernel.org> (raw)
In-Reply-To: <20260923024258.257846-6-kyrie.wu@mediatek.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] vdec: mtk_vcodec_vcp_dm_addr returns NULL violating API contract
- [High] vdec: missing map_dm_addr error validation in H264 and HEVC VCP init
- [High] vdec: concurrent VCP streams corrupt shared VSI memory
- [High] vdec: invalid pointer arithmetic on NULL vsi_core bypasses error checks
- [Medium] vdec: missing bounds validation on VCP shared memory offsets
--
commit d06f41499afd921668f031d62bb44ee4859ac008
Author: Kyrie Wu <kyrie.wu@mediatek.com>
media: mediatek: vcodec: get share memory address
This commit introduces shared memory division for the VCP architecture in the
MediaTek vcodec driver. It adds functions to compute different memory
addresses based on memory type for components like AV1, H264, HEVC, and VP9.
> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> index ece4e3e87e62d..468c4bf4e48ba 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> @@ -483,8 +483,31 @@ static unsigned int mtk_vcodec_vcp_get_vdec_capa(struct mtk_vcodec_fw *fw)
[ ... ]
> +static void *mtk_vcodec_vcp_dm_addr(struct mtk_vcodec_fw *fw, u32 mem_type)
> +{
> + unsigned char *vsi_core = fw->vcp->vsi_core_addr;
> +
> + switch (mem_type) {
> + case ENCODER_MEM:
> + case VCODEC_LAT_MEM:
> + return fw->vcp->vsi_addr;
[Severity: High]
Since vpu_dec_get_vsi_addr() requests VCODEC_LAT_MEM for all VCP decoding
instances, will returning the singular global fw->vcp->vsi_addr here cause
concurrent decoding contexts to overwrite each other's memory?
> + case VCODEC_CORE_MEM:
> + return vsi_core;
> + case VP9_FRAME_MEM:
> + return vsi_core + VCODEC_VSI_LEN;
[Severity: High]
Does this code perform invalid pointer arithmetic if vsi_core is NULL?
If the firmware failed to allocate and vsi_core_addr is NULL, adding
VCODEC_VSI_LEN results in a non-NULL invalid pointer (e.g., 0x2000) being
returned to callers.
> + case AV1_CDF_MEM:
> + return vsi_core + VCODEC_VSI_LEN + VP9_FRAME_SIZE;
> + case AV1_IQ_MEM:
> + return vsi_core + VCODEC_VSI_LEN + VP9_FRAME_SIZE + AV1_CDF_SIZE;
[Severity: Medium]
Is there a missing bounds validation on these shared memory offsets?
These offsets stack additively, requiring a minimum backing DMA buffer size.
If the hardware or firmware provisions a smaller memory block than expected,
this could silently return out-of-bounds DMA pointers, resulting in invalid
memory accesses when the codec later reads or writes these structures.
> + default:
> + break;
> + }
> +
> + return NULL;
> +}
[Severity: High]
Does returning NULL here violate the API contract expected by callers?
Other implementations return ERR_PTR(-EINVAL) on failure. Returning NULL
causes issues for callers like vdec_av1_slice_init_cdf_table() that expect to
validate the result using IS_ERR().
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
> index 756fbb7778b1f..4932ef4695946 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
> @@ -775,7 +784,7 @@ static int vdec_av1_slice_init_cdf_table(struct vdec_av1_slice_instance *instanc
> ctx = instance->ctx;
> vsi = instance->vpu.vsi;
> remote_cdf_table = mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler,
> - (u32)vsi->cdf_table_addr);
> + vdec_av1_get_cdf_table_addr(ctx, vsi));
> if (IS_ERR(remote_cdf_table)) {
> mtk_vdec_err(ctx, "failed to map cdf table\n");
> return PTR_ERR(remote_cdf_table);
> }
[Severity: High]
Will this IS_ERR() check fail to catch the NULL return from
mtk_vcodec_vcp_dm_addr()?
Since mtk_vcodec_vcp_dm_addr() returns NULL instead of an error pointer for
unhandled memory types, this IS_ERR() check evaluates to false. This allows
the code to proceed and attempt a memcpy() from a NULL remote_cdf_table.
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> index 69d60717181a6..544d3bc065645 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> @@ -1233,7 +1233,11 @@ static int vdec_h264_slice_init(struct mtk_vcodec_dec_ctx *ctx)
> vsi_size = round_up(vsi_size, VCODEC_DEC_ALIGNED_64);
> inst->vsi_ext = inst->vpu.vsi;
> temp = (unsigned char *)inst->vsi_ext;
> - inst->vsi_core_ext = (struct vdec_h264_slice_vsi_ext *)(temp + vsi_size);
> + if (mtk_vcodec_fw_get_type(ctx->dev->fw_handler) == VCP)
> + inst->vsi_core_ext =
> + mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler, VCODEC_CORE_MEM);
[Severity: High]
Are we missing an error check for the result of mtk_vcodec_fw_map_dm_addr()?
The mapped address is directly assigned to inst->vsi_core_ext without checking
for NULL or ERR_PTR. If the mapping fails, the invalid pointer will be stored
and later dereferenced unconditionally during core decode operations.
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c
> index dd638ef440836..a5dd42987452f 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c
> @@ -879,8 +879,11 @@ static int vdec_hevc_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>
> vsi_size = round_up(sizeof(struct vdec_hevc_slice_vsi), VCODEC_DEC_ALIGNED_64);
> inst->vsi = inst->vpu.vsi;
> - inst->vsi_core =
> - (struct vdec_hevc_slice_vsi *)(((char *)inst->vpu.vsi) + vsi_size);
> + if (mtk_vcodec_fw_get_type(ctx->dev->fw_handler) == VCP)
> + inst->vsi_core = mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler, VCODEC_CORE_MEM);
[Severity: High]
Does this code also need to validate the return value of
mtk_vcodec_fw_map_dm_addr() before assignment?
Similar to the H264 case, storing a potentially failed mapping without checking
for NULL or ERR_PTR could result in a kernel crash during decode operations.
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> index b492686784ec3..ee4d1cd82426f 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
> @@ -514,7 +523,7 @@ static int vdec_vp9_slice_init_default_frame_ctx(struct vdec_vp9_slice_instance
> return -EINVAL;
>
> remote_frame_ctx = mtk_vcodec_fw_map_dm_addr(ctx->dev->fw_handler,
> - (u32)vsi->default_frame_ctx);
> + vdec_vp9_get_frame_ctx_addr(ctx, vsi));
> if (!remote_frame_ctx) {
> mtk_vdec_err(ctx, "failed to map default frame ctx\n");
> return -EINVAL;
> }
[Severity: High]
Will this check fail to protect against an invalid pointer from
mtk_vcodec_vcp_dm_addr()?
If mtk_vcodec_vcp_dm_addr() returns an offset added to a NULL base (e.g.,
0x2000), this !remote_frame_ctx check will evaluate to false. This allows
execution to proceed and crash when kmemdup() later attempts to dereference
the invalid pointer.
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> index b35759a0b3533..cdb673e6b4770 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> @@ -9,6 +9,14 @@
> #include "vdec_ipi_msg.h"
> #include "vdec_vpu_if.h"
>
> +static u32 vpu_dec_get_vsi_addr(struct vdec_vpu_inst *vpu, const struct vdec_vpu_ipi_init_ack *msg)
> +{
> + if (mtk_vcodec_fw_get_type(vpu->ctx->dev->fw_handler) == VCP)
> + return VCODEC_LAT_MEM;
> + else
> + return msg->vpu_inst_addr;
> +}
[Severity: High]
Can returning a single hardcoded enum for all VCP instances lead to memory
corruption?
By requesting VCODEC_LAT_MEM for all VCP decoding instances, multiple
concurrent V4L2 mem2mem decoding contexts will be mapped to the exact same
global VSI memory buffer. This means concurrent streams could silently
overwrite each other's shared state.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260923024258.257846-1-kyrie.wu@mediatek.com?part=5
next prev parent reply other threads:[~2026-09-23 2:58 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-23 2:42 [PATCH v7 00/14] media: mediatek: vcodec: support video decoder in mt8196 Kyrie Wu
2026-09-23 2:42 ` [PATCH v7 01/14] media: mediatek: vcodec: support vcp architecture Kyrie Wu
2026-09-23 2:42 ` [PATCH v7 02/14] media: mediatek: vcodec: add driver to support vcp Kyrie Wu
2026-09-23 2:57 ` sashiko-bot
2026-09-23 2:42 ` [PATCH v7 03/14] media: mediatek: vcodec: add driver to support vcp encoder Kyrie Wu
2026-09-23 3:06 ` sashiko-bot
2026-09-23 2:42 ` [PATCH v7 04/14] media: mediatek: vcodec: get different firmware ipi id Kyrie Wu
2026-09-23 3:03 ` sashiko-bot
2026-09-23 2:42 ` [PATCH v7 05/14] media: mediatek: vcodec: get share memory address Kyrie Wu
2026-09-23 2:58 ` sashiko-bot [this message]
2026-09-23 2:42 ` [PATCH v7 06/14] media: mediatek: vcodec: add debug information Kyrie Wu
2026-09-23 2:42 ` [PATCH v7 07/14] media: mediatek: vcodec: send share memory address to vcp Kyrie Wu
2026-09-23 2:42 ` [PATCH v7 08/14] Add the MT8196 compatible string to the MediaTek vcodec subdev decoder binding Kyrie Wu
2026-10-06 14:35 ` Rob Herring
2026-09-23 2:42 ` [PATCH v7 09/14] media: mediatek: vcodec: add decoder compatible to support mt8196 Kyrie Wu
2026-09-23 2:42 ` [PATCH v7 10/14] media: mediatek: vcodec: define MT8196 vcodec levels Kyrie Wu
2026-09-23 2:42 ` [PATCH v7 11/14] media: mediatek: vcodec: support 36bit iova address Kyrie Wu
2026-09-23 2:42 ` [PATCH v7 12/14] media: mediatek: vcodec: clean xpc status Kyrie Wu
2026-09-23 3:02 ` sashiko-bot
2026-09-23 2:42 ` [PATCH v7 13/14] media: mediatek: decoder: fill av1 buffer size with picinfo Kyrie Wu
2026-09-23 2:42 ` [PATCH v7 14/14] media: mediatek: decoder: support av1 extend vsi Kyrie Wu
2026-09-23 3:00 ` sashiko-bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260923025843.0398C1F00893@smtp.kernel.org \
--to=sashiko-bot@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=kyrie.wu@mediatek.com \
--cc=media-ci@linuxtv.org \
--cc=robh@kernel.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox