[v3,1/2] media: mediatek: vcodec: Fix possible invalid memory access for decoder
Commit Message
The vpu maybe null pointer or unreasonable value when scp crash, need to
validate that the vpu pointer and the vpu instance within this context is
valid in case of leading to kernel reboot.
Fixes: 590577a4e525 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver")
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
Reported-by: Steve Cho <stevecho@google.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
- compared with v2:
- rewrite the commit message for patch 01 and 02.
- add Reported-by and Fixes tag.
- fix smatch fail for patch 02/2.
---
.../vcodec/decoder/mtk_vcodec_dec_drv.h | 2 +
.../mediatek/vcodec/decoder/vdec_vpu_if.c | 77 ++++++++++++-------
2 files changed, 52 insertions(+), 27 deletions(-)
Comments
Hi Yunfei,
On 29/07/2023 05:47, Yunfei Dong wrote:
> The vpu maybe null pointer or unreasonable value when scp crash, need to
> validate that the vpu pointer and the vpu instance within this context is
> valid in case of leading to kernel reboot.
>
> Fixes: 590577a4e525 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver")
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> Reported-by: Steve Cho <stevecho@google.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
> - compared with v2:
> - rewrite the commit message for patch 01 and 02.
> - add Reported-by and Fixes tag.
> - fix smatch fail for patch 02/2.
> ---
> .../vcodec/decoder/mtk_vcodec_dec_drv.h | 2 +
> .../mediatek/vcodec/decoder/vdec_vpu_if.c | 77 ++++++++++++-------
> 2 files changed, 52 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> index 6c318de25a55e..7e36b2c69b7d1 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> @@ -161,6 +161,7 @@ struct mtk_vcodec_dec_pdata {
> * @hw_id: hardware index used to identify different hardware.
> *
> * @msg_queue: msg queue used to store lat buffer information.
> + * @vpu_inst: vpu instance pointer.
> *
> * @is_10bit_bitstream: set to true if it's 10bit bitstream
> */
> @@ -205,6 +206,7 @@ struct mtk_vcodec_dec_ctx {
> int hw_id;
>
> struct vdec_msg_queue msg_queue;
> + void *vpu_inst;
>
> bool is_10bit_bitstream;
> };
> 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 82c3dc8c41273..23cfe5c6c90b7 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> @@ -72,6 +72,21 @@ static void handle_get_param_msg_ack(const struct vdec_vpu_ipi_get_param_ack *ms
> }
> }
>
> +static bool vpu_dec_check_ap_inst(struct mtk_vcodec_dec_dev *dec_dev, struct vdec_vpu_inst *vpu)
> +{
> + struct mtk_vcodec_dec_ctx *ctx;
> + int ret = false;
> +
> + list_for_each_entry(ctx, &dec_dev->ctx_list, list) {
> + if (!IS_ERR_OR_NULL(ctx) && ctx->vpu_inst == vpu) {
> + ret = true;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> /*
> * vpu_dec_ipi_handler - Handler for VPU ipi message.
> *
> @@ -84,44 +99,51 @@ static void handle_get_param_msg_ack(const struct vdec_vpu_ipi_get_param_ack *ms
> */
> static void vpu_dec_ipi_handler(void *data, unsigned int len, void *priv)
> {
> + struct mtk_vcodec_dec_dev *dec_dev;
> const struct vdec_vpu_ipi_ack *msg = data;
> - struct vdec_vpu_inst *vpu = (struct vdec_vpu_inst *)
> - (unsigned long)msg->ap_inst_addr;
> + struct vdec_vpu_inst *vpu;
>
> - if (!vpu) {
> + dec_dev = (struct mtk_vcodec_dec_dev *)priv;
> + vpu = (struct vdec_vpu_inst *)(unsigned long)msg->ap_inst_addr;
> + if (!priv || !vpu) {
> mtk_v4l2_vdec_err(vpu->ctx, "ap_inst_addr is NULL, did the SCP hang or crash?");
> return;
> }
This change results in a smatch error:
drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c:109 vpu_dec_ipi_handler() error: we previously assumed 'vpu' could be null (see line 108)
The same happens in the next patch for vpu_enc_ipi_handler.
Can you post a v4 fixing this?
Thank you!
Hans
>
> - mtk_vdec_debug(vpu->ctx, "+ id=%X", msg->msg_id);
> + if (!vpu_dec_check_ap_inst(dec_dev, vpu) || msg->msg_id < VPU_IPIMSG_DEC_INIT_ACK ||
> + msg->msg_id > VPU_IPIMSG_DEC_GET_PARAM_ACK) {
> + mtk_v4l2_vdec_err(vpu->ctx, "vdec msg id not correctly => 0x%x", msg->msg_id);
> + vpu->failure = -EINVAL;
> + goto error;
> + }
>
> vpu->failure = msg->status;
> - vpu->signaled = 1;
> + if (msg->status != 0)
> + goto error;
>
> - if (msg->status == 0) {
> - switch (msg->msg_id) {
> - case VPU_IPIMSG_DEC_INIT_ACK:
> - handle_init_ack_msg(data);
> - break;
> + switch (msg->msg_id) {
> + case VPU_IPIMSG_DEC_INIT_ACK:
> + handle_init_ack_msg(data);
> + break;
>
> - case VPU_IPIMSG_DEC_START_ACK:
> - case VPU_IPIMSG_DEC_END_ACK:
> - case VPU_IPIMSG_DEC_DEINIT_ACK:
> - case VPU_IPIMSG_DEC_RESET_ACK:
> - case VPU_IPIMSG_DEC_CORE_ACK:
> - case VPU_IPIMSG_DEC_CORE_END_ACK:
> - break;
> + case VPU_IPIMSG_DEC_START_ACK:
> + case VPU_IPIMSG_DEC_END_ACK:
> + case VPU_IPIMSG_DEC_DEINIT_ACK:
> + case VPU_IPIMSG_DEC_RESET_ACK:
> + case VPU_IPIMSG_DEC_CORE_ACK:
> + case VPU_IPIMSG_DEC_CORE_END_ACK:
> + break;
>
> - case VPU_IPIMSG_DEC_GET_PARAM_ACK:
> - handle_get_param_msg_ack(data);
> - break;
> - default:
> - mtk_vdec_err(vpu->ctx, "invalid msg=%X", msg->msg_id);
> - break;
> - }
> + case VPU_IPIMSG_DEC_GET_PARAM_ACK:
> + handle_get_param_msg_ack(data);
> + break;
> + default:
> + mtk_vdec_err(vpu->ctx, "invalid msg=%X", msg->msg_id);
> + break;
> }
>
> - mtk_vdec_debug(vpu->ctx, "- id=%X", msg->msg_id);
> +error:
> + vpu->signaled = 1;
> }
>
> static int vcodec_vpu_send_msg(struct vdec_vpu_inst *vpu, void *msg, int len)
> @@ -182,9 +204,10 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
>
> init_waitqueue_head(&vpu->wq);
> vpu->handler = vpu_dec_ipi_handler;
> + vpu->ctx->vpu_inst = vpu;
>
> err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu->id,
> - vpu->handler, "vdec", NULL);
> + vpu->handler, "vdec", vpu->ctx->dev);
> if (err) {
> mtk_vdec_err(vpu->ctx, "vpu_ipi_register fail status=%d", err);
> return err;
> @@ -193,7 +216,7 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
> if (vpu->ctx->dev->vdec_pdata->hw_arch == MTK_VDEC_LAT_SINGLE_CORE) {
> err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler,
> vpu->core_id, vpu->handler,
> - "vdec", NULL);
> + "vdec", vpu->ctx->dev);
> if (err) {
> mtk_vdec_err(vpu->ctx, "vpu_ipi_register core fail status=%d", err);
> return err;
Hi hans,
Thanks for your test and review.
On Wed, 2023-08-02 at 10:51 +0200, Hans Verkuil wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Hi Yunfei,
>
> On 29/07/2023 05:47, Yunfei Dong wrote:
> > The vpu maybe null pointer or unreasonable value when scp crash,
> need to
> > validate that the vpu pointer and the vpu instance within this
> context is
> > valid in case of leading to kernel reboot.
> >
> > Fixes: 590577a4e525 ("[media] vcodec: mediatek: Add Mediatek V4L2
> Video Decoder Driver")
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > Reported-by: Steve Cho <stevecho@google.com>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> > - compared with v2:
> > - rewrite the commit message for patch 01 and 02.
> > - add Reported-by and Fixes tag.
> > - fix smatch fail for patch 02/2.
> > ---
> > .../vcodec/decoder/mtk_vcodec_dec_drv.h | 2 +
> > .../mediatek/vcodec/decoder/vdec_vpu_if.c | 77 ++++++++++++---
> ----
> > 2 files changed, 52 insertions(+), 27 deletions(-)
> >
> > diff --git
> a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> > index 6c318de25a55e..7e36b2c69b7d1 100644
> > ---
> a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> > +++
> b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> > @@ -161,6 +161,7 @@ struct mtk_vcodec_dec_pdata {
> > * @hw_id: hardware index used to identify different hardware.
> > *
> > * @msg_queue: msg queue used to store lat buffer information.
> > + * @vpu_inst: vpu instance pointer.
> > *
> > * @is_10bit_bitstream: set to true if it's 10bit bitstream
> > */
> > @@ -205,6 +206,7 @@ struct mtk_vcodec_dec_ctx {
> > int hw_id;
> >
> > struct vdec_msg_queue msg_queue;
> > +void *vpu_inst;
> >
> > bool is_10bit_bitstream;
> > };
> > 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 82c3dc8c41273..23cfe5c6c90b7 100644
> > --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > @@ -72,6 +72,21 @@ static void handle_get_param_msg_ack(const
> struct vdec_vpu_ipi_get_param_ack *ms
> > }
> > }
> >
> > +static bool vpu_dec_check_ap_inst(struct mtk_vcodec_dec_dev
> *dec_dev, struct vdec_vpu_inst *vpu)
> > +{
> > +struct mtk_vcodec_dec_ctx *ctx;
> > +int ret = false;
> > +
> > +list_for_each_entry(ctx, &dec_dev->ctx_list, list) {
> > +if (!IS_ERR_OR_NULL(ctx) && ctx->vpu_inst == vpu) {
> > +ret = true;
> > +break;
> > +}
> > +}
> > +
> > +return ret;
> > +}
> > +
> > /*
> > * vpu_dec_ipi_handler - Handler for VPU ipi message.
> > *
> > @@ -84,44 +99,51 @@ static void handle_get_param_msg_ack(const
> struct vdec_vpu_ipi_get_param_ack *ms
> > */
> > static void vpu_dec_ipi_handler(void *data, unsigned int len, void
> *priv)
> > {
> > +struct mtk_vcodec_dec_dev *dec_dev;
> > const struct vdec_vpu_ipi_ack *msg = data;
> > -struct vdec_vpu_inst *vpu = (struct vdec_vpu_inst *)
> > -(unsigned long)msg->ap_inst_addr;
> > +struct vdec_vpu_inst *vpu;
> >
> > -if (!vpu) {
> > +dec_dev = (struct mtk_vcodec_dec_dev *)priv;
> > +vpu = (struct vdec_vpu_inst *)(unsigned long)msg->ap_inst_addr;
> > +if (!priv || !vpu) {
> > mtk_v4l2_vdec_err(vpu->ctx, "ap_inst_addr is NULL, did the SCP
> hang or crash?");
> > return;
> > }
>
> This change results in a smatch error:
>
> drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c:109
> vpu_dec_ipi_handler() error: we previously assumed 'vpu' could be
> null (see line 108)
>
> The same happens in the next patch for vpu_enc_ipi_handler.
>
> Can you post a v4 fixing this?
>
> Thank you!
>
> Hans
Please help to review v4 again.
Best Regards,
Yunfei Dong
>
> >
> > -mtk_vdec_debug(vpu->ctx, "+ id=%X", msg->msg_id);
> > +if (!vpu_dec_check_ap_inst(dec_dev, vpu) || msg->msg_id <
> VPU_IPIMSG_DEC_INIT_ACK ||
> > + msg->msg_id > VPU_IPIMSG_DEC_GET_PARAM_ACK) {
> > +mtk_v4l2_vdec_err(vpu->ctx, "vdec msg id not correctly => 0x%x",
> msg->msg_id);
> > +vpu->failure = -EINVAL;
> > +goto error;
> > +}
> >
> > vpu->failure = msg->status;
> > -vpu->signaled = 1;
> > +if (msg->status != 0)
> > +goto error;
> >
> > -if (msg->status == 0) {
> > -switch (msg->msg_id) {
> > -case VPU_IPIMSG_DEC_INIT_ACK:
> > -handle_init_ack_msg(data);
> > -break;
> > +switch (msg->msg_id) {
> > +case VPU_IPIMSG_DEC_INIT_ACK:
> > +handle_init_ack_msg(data);
> > +break;
> >
> > -case VPU_IPIMSG_DEC_START_ACK:
> > -case VPU_IPIMSG_DEC_END_ACK:
> > -case VPU_IPIMSG_DEC_DEINIT_ACK:
> > -case VPU_IPIMSG_DEC_RESET_ACK:
> > -case VPU_IPIMSG_DEC_CORE_ACK:
> > -case VPU_IPIMSG_DEC_CORE_END_ACK:
> > -break;
> > +case VPU_IPIMSG_DEC_START_ACK:
> > +case VPU_IPIMSG_DEC_END_ACK:
> > +case VPU_IPIMSG_DEC_DEINIT_ACK:
> > +case VPU_IPIMSG_DEC_RESET_ACK:
> > +case VPU_IPIMSG_DEC_CORE_ACK:
> > +case VPU_IPIMSG_DEC_CORE_END_ACK:
> > +break;
> >
> > -case VPU_IPIMSG_DEC_GET_PARAM_ACK:
> > -handle_get_param_msg_ack(data);
> > -break;
> > -default:
> > -mtk_vdec_err(vpu->ctx, "invalid msg=%X", msg->msg_id);
> > -break;
> > -}
> > +case VPU_IPIMSG_DEC_GET_PARAM_ACK:
> > +handle_get_param_msg_ack(data);
> > +break;
> > +default:
> > +mtk_vdec_err(vpu->ctx, "invalid msg=%X", msg->msg_id);
> > +break;
> > }
> >
> > -mtk_vdec_debug(vpu->ctx, "- id=%X", msg->msg_id);
> > +error:
> > +vpu->signaled = 1;
> > }
> >
> > static int vcodec_vpu_send_msg(struct vdec_vpu_inst *vpu, void
> *msg, int len)
> > @@ -182,9 +204,10 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
> >
> > init_waitqueue_head(&vpu->wq);
> > vpu->handler = vpu_dec_ipi_handler;
> > +vpu->ctx->vpu_inst = vpu;
> >
> > err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu-
> >id,
> > - vpu->handler, "vdec", NULL);
> > + vpu->handler, "vdec", vpu->ctx->dev);
> > if (err) {
> > mtk_vdec_err(vpu->ctx, "vpu_ipi_register fail status=%d", err);
> > return err;
> > @@ -193,7 +216,7 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
> > if (vpu->ctx->dev->vdec_pdata->hw_arch ==
> MTK_VDEC_LAT_SINGLE_CORE) {
> > err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler,
> > vpu->core_id, vpu->handler,
> > - "vdec", NULL);
> > + "vdec", vpu->ctx->dev);
> > if (err) {
> > mtk_vdec_err(vpu->ctx, "vpu_ipi_register core fail status=%d",
> err);
> > return err;
>
>
@@ -161,6 +161,7 @@ struct mtk_vcodec_dec_pdata {
* @hw_id: hardware index used to identify different hardware.
*
* @msg_queue: msg queue used to store lat buffer information.
+ * @vpu_inst: vpu instance pointer.
*
* @is_10bit_bitstream: set to true if it's 10bit bitstream
*/
@@ -205,6 +206,7 @@ struct mtk_vcodec_dec_ctx {
int hw_id;
struct vdec_msg_queue msg_queue;
+ void *vpu_inst;
bool is_10bit_bitstream;
};
@@ -72,6 +72,21 @@ static void handle_get_param_msg_ack(const struct vdec_vpu_ipi_get_param_ack *ms
}
}
+static bool vpu_dec_check_ap_inst(struct mtk_vcodec_dec_dev *dec_dev, struct vdec_vpu_inst *vpu)
+{
+ struct mtk_vcodec_dec_ctx *ctx;
+ int ret = false;
+
+ list_for_each_entry(ctx, &dec_dev->ctx_list, list) {
+ if (!IS_ERR_OR_NULL(ctx) && ctx->vpu_inst == vpu) {
+ ret = true;
+ break;
+ }
+ }
+
+ return ret;
+}
+
/*
* vpu_dec_ipi_handler - Handler for VPU ipi message.
*
@@ -84,44 +99,51 @@ static void handle_get_param_msg_ack(const struct vdec_vpu_ipi_get_param_ack *ms
*/
static void vpu_dec_ipi_handler(void *data, unsigned int len, void *priv)
{
+ struct mtk_vcodec_dec_dev *dec_dev;
const struct vdec_vpu_ipi_ack *msg = data;
- struct vdec_vpu_inst *vpu = (struct vdec_vpu_inst *)
- (unsigned long)msg->ap_inst_addr;
+ struct vdec_vpu_inst *vpu;
- if (!vpu) {
+ dec_dev = (struct mtk_vcodec_dec_dev *)priv;
+ vpu = (struct vdec_vpu_inst *)(unsigned long)msg->ap_inst_addr;
+ if (!priv || !vpu) {
mtk_v4l2_vdec_err(vpu->ctx, "ap_inst_addr is NULL, did the SCP hang or crash?");
return;
}
- mtk_vdec_debug(vpu->ctx, "+ id=%X", msg->msg_id);
+ if (!vpu_dec_check_ap_inst(dec_dev, vpu) || msg->msg_id < VPU_IPIMSG_DEC_INIT_ACK ||
+ msg->msg_id > VPU_IPIMSG_DEC_GET_PARAM_ACK) {
+ mtk_v4l2_vdec_err(vpu->ctx, "vdec msg id not correctly => 0x%x", msg->msg_id);
+ vpu->failure = -EINVAL;
+ goto error;
+ }
vpu->failure = msg->status;
- vpu->signaled = 1;
+ if (msg->status != 0)
+ goto error;
- if (msg->status == 0) {
- switch (msg->msg_id) {
- case VPU_IPIMSG_DEC_INIT_ACK:
- handle_init_ack_msg(data);
- break;
+ switch (msg->msg_id) {
+ case VPU_IPIMSG_DEC_INIT_ACK:
+ handle_init_ack_msg(data);
+ break;
- case VPU_IPIMSG_DEC_START_ACK:
- case VPU_IPIMSG_DEC_END_ACK:
- case VPU_IPIMSG_DEC_DEINIT_ACK:
- case VPU_IPIMSG_DEC_RESET_ACK:
- case VPU_IPIMSG_DEC_CORE_ACK:
- case VPU_IPIMSG_DEC_CORE_END_ACK:
- break;
+ case VPU_IPIMSG_DEC_START_ACK:
+ case VPU_IPIMSG_DEC_END_ACK:
+ case VPU_IPIMSG_DEC_DEINIT_ACK:
+ case VPU_IPIMSG_DEC_RESET_ACK:
+ case VPU_IPIMSG_DEC_CORE_ACK:
+ case VPU_IPIMSG_DEC_CORE_END_ACK:
+ break;
- case VPU_IPIMSG_DEC_GET_PARAM_ACK:
- handle_get_param_msg_ack(data);
- break;
- default:
- mtk_vdec_err(vpu->ctx, "invalid msg=%X", msg->msg_id);
- break;
- }
+ case VPU_IPIMSG_DEC_GET_PARAM_ACK:
+ handle_get_param_msg_ack(data);
+ break;
+ default:
+ mtk_vdec_err(vpu->ctx, "invalid msg=%X", msg->msg_id);
+ break;
}
- mtk_vdec_debug(vpu->ctx, "- id=%X", msg->msg_id);
+error:
+ vpu->signaled = 1;
}
static int vcodec_vpu_send_msg(struct vdec_vpu_inst *vpu, void *msg, int len)
@@ -182,9 +204,10 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
init_waitqueue_head(&vpu->wq);
vpu->handler = vpu_dec_ipi_handler;
+ vpu->ctx->vpu_inst = vpu;
err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu->id,
- vpu->handler, "vdec", NULL);
+ vpu->handler, "vdec", vpu->ctx->dev);
if (err) {
mtk_vdec_err(vpu->ctx, "vpu_ipi_register fail status=%d", err);
return err;
@@ -193,7 +216,7 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
if (vpu->ctx->dev->vdec_pdata->hw_arch == MTK_VDEC_LAT_SINGLE_CORE) {
err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler,
vpu->core_id, vpu->handler,
- "vdec", NULL);
+ "vdec", vpu->ctx->dev);
if (err) {
mtk_vdec_err(vpu->ctx, "vpu_ipi_register core fail status=%d", err);
return err;