diff mbox series

media: mediatek: vcodec: Fix oops when HEVC init fails

Message ID 20240226211954.400891-1-nicolas.dufresne@collabora.com (mailing list archive)
State New, archived
Headers show
Series media: mediatek: vcodec: Fix oops when HEVC init fails | expand

Commit Message

Nicolas Dufresne Feb. 26, 2024, 9:19 p.m. UTC 
In stateless HEVC case, the instance pointer was saved in the
context regardless if the initialization worked. As the pointer
is freed in failure, this resulted in use after free in the
deinit function.

 Hardware name: Acer Tomato (rev3 - 4) board (DT)
 pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : vcodec_vpu_send_msg+0x4c/0x190 [mtk_vcodec_dec]
 lr : vcodec_send_ap_ipi+0x78/0x170 [mtk_vcodec_dec]
 sp : ffff80008750bc20
 x29: ffff80008750bc20 x28: ffff1299f6d70000 x27: 0000000000000000
 x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
 x23: ffff80008750bc98 x22: 000000000000a003 x21: ffffd45c4cfae000
 x20: 0000000000000010 x19: ffff1299fd668310 x18: 000000000000001a
 x17: 000000040044ffff x16: ffffd45cb15dc648 x15: 0000000000000000
 x14: ffff1299c08da1c0 x13: ffffd45cb1f87a10 x12: ffffd45cb2f5fe80
 x11: 0000000000000001 x10: 0000000000001b30 x9 : ffffd45c4d12b488
 x8 : 1fffe25339380d81 x7 : 0000000000000001 x6 : ffff1299c9c06c00
 x5 : 0000000000000132 x4 : 0000000000000000 x3 : 0000000000000000
 x2 : 0000000000000010 x1 : ffff80008750bc98 x0 : 0000000000000000
 Call trace:
  vcodec_vpu_send_msg+0x4c/0x190 [mtk_vcodec_dec]
  vcodec_send_ap_ipi+0x78/0x170 [mtk_vcodec_dec]
  vpu_dec_deinit+0x1c/0x30 [mtk_vcodec_dec]
  vdec_hevc_slice_deinit+0x30/0x98 [mtk_vcodec_dec]
  vdec_if_deinit+0x38/0x68 [mtk_vcodec_dec]
  mtk_vcodec_dec_release+0x20/0x40 [mtk_vcodec_dec]
  fops_vcodec_release+0x64/0x118 [mtk_vcodec_dec]
  v4l2_release+0x7c/0x100
  __fput+0x80/0x2d8
  __fput_sync+0x58/0x70
  __arm64_sys_close+0x40/0x90
  invoke_syscall+0x50/0x128
  el0_svc_common.constprop.0+0x48/0xf0
  do_el0_svc+0x24/0x38
  el0_svc+0x38/0xd8
  el0t_64_sync_handler+0xc0/0xc8
  el0t_64_sync+0x1a8/0x1b0
 Code: d503201f f9401660 b900127f b900227f (f9400400)

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Fixes: 2674486aac7d ("media: mediatek: vcodec: support stateless hevc decoder")
---
 .../mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c       | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

AngeloGioacchino Del Regno Feb. 28, 2024, 10:09 a.m. UTC | #1 
Il 26/02/24 22:19, Nicolas Dufresne ha scritto:
> In stateless HEVC case, the instance pointer was saved in the
> context regardless if the initialization worked. As the pointer
> is freed in failure, this resulted in use after free in the
> deinit function.
> 

 From what I understand, "the pointer" that is freed is struct vdec_vpu_inst; is
that correct?

Since you do have a way to easily reproduce the issue on your side, can we resolve
the safety/reliability root issue as well as making this correct?

The idea is being able to avoid a kernel panic in the situation that you describe
in this fix, but throw an error message (read: throw a big "wtf!") when this does
happen, and handle that with returning an error code (avoiding a kernel crash).

Let's cut it short - please try this:

In functions
   - int vpu_vdec_start()
   - int vpu_dec_get_param()
   - int vcodec_send_ap_ipi()

at the beginning, perform this check:

	if (unlikely(!vpu)) {
		/* Write a scarier message if this is not scary enough */
		mtk_vdec_err("FIXME!! - VPU is NULL. This is unexpected.\n");
		return -EINVAL; /* or something else if more meaningful */
	}

Unless I've misunderstood what's NULL, that'll work. :-)


Meanwhile, for this fix:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers,
Angelo

>   Hardware name: Acer Tomato (rev3 - 4) board (DT)
>   pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : vcodec_vpu_send_msg+0x4c/0x190 [mtk_vcodec_dec]
>   lr : vcodec_send_ap_ipi+0x78/0x170 [mtk_vcodec_dec]
>   sp : ffff80008750bc20
>   x29: ffff80008750bc20 x28: ffff1299f6d70000 x27: 0000000000000000
>   x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
>   x23: ffff80008750bc98 x22: 000000000000a003 x21: ffffd45c4cfae000
>   x20: 0000000000000010 x19: ffff1299fd668310 x18: 000000000000001a
>   x17: 000000040044ffff x16: ffffd45cb15dc648 x15: 0000000000000000
>   x14: ffff1299c08da1c0 x13: ffffd45cb1f87a10 x12: ffffd45cb2f5fe80
>   x11: 0000000000000001 x10: 0000000000001b30 x9 : ffffd45c4d12b488
>   x8 : 1fffe25339380d81 x7 : 0000000000000001 x6 : ffff1299c9c06c00
>   x5 : 0000000000000132 x4 : 0000000000000000 x3 : 0000000000000000
>   x2 : 0000000000000010 x1 : ffff80008750bc98 x0 : 0000000000000000
>   Call trace:
>    vcodec_vpu_send_msg+0x4c/0x190 [mtk_vcodec_dec]
>    vcodec_send_ap_ipi+0x78/0x170 [mtk_vcodec_dec]
>    vpu_dec_deinit+0x1c/0x30 [mtk_vcodec_dec]
>    vdec_hevc_slice_deinit+0x30/0x98 [mtk_vcodec_dec]
>    vdec_if_deinit+0x38/0x68 [mtk_vcodec_dec]
>    mtk_vcodec_dec_release+0x20/0x40 [mtk_vcodec_dec]
>    fops_vcodec_release+0x64/0x118 [mtk_vcodec_dec]
>    v4l2_release+0x7c/0x100
>    __fput+0x80/0x2d8
>    __fput_sync+0x58/0x70
>    __arm64_sys_close+0x40/0x90
>    invoke_syscall+0x50/0x128
>    el0_svc_common.constprop.0+0x48/0xf0
>    do_el0_svc+0x24/0x38
>    el0_svc+0x38/0xd8
>    el0t_64_sync_handler+0xc0/0xc8
>    el0t_64_sync+0x1a8/0x1b0
>   Code: d503201f f9401660 b900127f b900227f (f9400400)
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Fixes: 2674486aac7d ("media: mediatek: vcodec: support stateless hevc decoder")
> ---
>   .../mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c       | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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 06ed47df693bf..21836dd6ef85a 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
> @@ -869,7 +869,6 @@ static int vdec_hevc_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>   	inst->vpu.codec_type = ctx->current_codec;
>   	inst->vpu.capture_type = ctx->capture_fourcc;
>   
> -	ctx->drv_handle = inst;
>   	err = vpu_dec_init(&inst->vpu);
>   	if (err) {
>   		mtk_vdec_err(ctx, "vdec_hevc init err=%d", err);
> @@ -898,6 +897,7 @@ static int vdec_hevc_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>   	mtk_vdec_debug(ctx, "lat hevc instance >> %p, codec_type = 0x%x",
>   		       inst, inst->vpu.codec_type);
>   
> +	ctx->drv_handle = inst;
>   	return 0;
>   error_free_inst:
>   	kfree(inst);
Sebastian Fricke March 13, 2024, 2 p.m. UTC | #2 
Hey Nicolas,

On 26.02.2024 16:19, Nicolas Dufresne wrote:
>In stateless HEVC case, the instance pointer was saved in the
>context regardless if the initialization worked. As the pointer
>is freed in failure, this resulted in use after free in the
>deinit function.

just to fix some grammatic erros in the commit log, my suggestion:

```
The stateless HEVC decoder saves the instance pointer in the context
regardless if the initialization worked or not. This caused a use after
free, when the pointer is freed in case of a failure in the deinit
function.
Only store the instance pointer when the initialization was successful,
to solve this issue.
```

Greetings,
Sebastian

>
> Hardware name: Acer Tomato (rev3 - 4) board (DT)
> pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : vcodec_vpu_send_msg+0x4c/0x190 [mtk_vcodec_dec]
> lr : vcodec_send_ap_ipi+0x78/0x170 [mtk_vcodec_dec]
> sp : ffff80008750bc20
> x29: ffff80008750bc20 x28: ffff1299f6d70000 x27: 0000000000000000
> x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
> x23: ffff80008750bc98 x22: 000000000000a003 x21: ffffd45c4cfae000
> x20: 0000000000000010 x19: ffff1299fd668310 x18: 000000000000001a
> x17: 000000040044ffff x16: ffffd45cb15dc648 x15: 0000000000000000
> x14: ffff1299c08da1c0 x13: ffffd45cb1f87a10 x12: ffffd45cb2f5fe80
> x11: 0000000000000001 x10: 0000000000001b30 x9 : ffffd45c4d12b488
> x8 : 1fffe25339380d81 x7 : 0000000000000001 x6 : ffff1299c9c06c00
> x5 : 0000000000000132 x4 : 0000000000000000 x3 : 0000000000000000
> x2 : 0000000000000010 x1 : ffff80008750bc98 x0 : 0000000000000000
> Call trace:
>  vcodec_vpu_send_msg+0x4c/0x190 [mtk_vcodec_dec]
>  vcodec_send_ap_ipi+0x78/0x170 [mtk_vcodec_dec]
>  vpu_dec_deinit+0x1c/0x30 [mtk_vcodec_dec]
>  vdec_hevc_slice_deinit+0x30/0x98 [mtk_vcodec_dec]
>  vdec_if_deinit+0x38/0x68 [mtk_vcodec_dec]
>  mtk_vcodec_dec_release+0x20/0x40 [mtk_vcodec_dec]
>  fops_vcodec_release+0x64/0x118 [mtk_vcodec_dec]
>  v4l2_release+0x7c/0x100
>  __fput+0x80/0x2d8
>  __fput_sync+0x58/0x70
>  __arm64_sys_close+0x40/0x90
>  invoke_syscall+0x50/0x128
>  el0_svc_common.constprop.0+0x48/0xf0
>  do_el0_svc+0x24/0x38
>  el0_svc+0x38/0xd8
>  el0t_64_sync_handler+0xc0/0xc8
>  el0t_64_sync+0x1a8/0x1b0
> Code: d503201f f9401660 b900127f b900227f (f9400400)
>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>Fixes: 2674486aac7d ("media: mediatek: vcodec: support stateless hevc decoder")
>---
> .../mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c       | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>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 06ed47df693bf..21836dd6ef85a 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
>@@ -869,7 +869,6 @@ static int vdec_hevc_slice_init(struct mtk_vcodec_dec_ctx *ctx)
> 	inst->vpu.codec_type = ctx->current_codec;
> 	inst->vpu.capture_type = ctx->capture_fourcc;
>
>-	ctx->drv_handle = inst;
> 	err = vpu_dec_init(&inst->vpu);
> 	if (err) {
> 		mtk_vdec_err(ctx, "vdec_hevc init err=%d", err);
>@@ -898,6 +897,7 @@ static int vdec_hevc_slice_init(struct mtk_vcodec_dec_ctx *ctx)
> 	mtk_vdec_debug(ctx, "lat hevc instance >> %p, codec_type = 0x%x",
> 		       inst, inst->vpu.codec_type);
>
>+	ctx->drv_handle = inst;
> 	return 0;
> error_free_inst:
> 	kfree(inst);
>-- 
>2.43.2
>
>_______________________________________________
>Kernel mailing list -- kernel@mailman.collabora.com
>To unsubscribe send an email to kernel-leave@mailman.collabora.com
>This list is managed by https://mailman.collabora.com
diff mbox series

Patch

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 06ed47df693bf..21836dd6ef85a 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
@@ -869,7 +869,6 @@  static int vdec_hevc_slice_init(struct mtk_vcodec_dec_ctx *ctx)
 	inst->vpu.codec_type = ctx->current_codec;
 	inst->vpu.capture_type = ctx->capture_fourcc;
 
-	ctx->drv_handle = inst;
 	err = vpu_dec_init(&inst->vpu);
 	if (err) {
 		mtk_vdec_err(ctx, "vdec_hevc init err=%d", err);
@@ -898,6 +897,7 @@  static int vdec_hevc_slice_init(struct mtk_vcodec_dec_ctx *ctx)
 	mtk_vdec_debug(ctx, "lat hevc instance >> %p, codec_type = 0x%x",
 		       inst, inst->vpu.codec_type);
 
+	ctx->drv_handle = inst;
 	return 0;
 error_free_inst:
 	kfree(inst);