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 <[email protected]>
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
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 <[email protected]>
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 <[email protected]>
> 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);
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 <[email protected]>
>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 -- [email protected]
>To unsubscribe send an email to [email protected]
>This list is managed by https://mailman.collabora.com