Need to set the private data with encoder device, or will access
NULL pointer in encoder handler.
Fixes: 1972e32431ed ("media: mediatek: vcodec: Fix possible invalid memory access for encoder")
Signed-off-by: Irui Wang <[email protected]>
---
drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
index d299cc2962a5..ae6290d28f8e 100644
--- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
+++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
@@ -138,7 +138,8 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
vpu->ctx->vpu_inst = vpu;
status = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu->id,
- vpu_enc_ipi_handler, "venc", NULL);
+ vpu_enc_ipi_handler, "venc",
+ vpu->ctx->dev);
if (status) {
mtk_venc_err(vpu->ctx, "vpu_ipi_register fail %d", status);
--
2.25.1
There will be a kernel null pointer exception if 'vsi' is NULL, check
'vsi' is not NULL before assign it to encoder instance.
Signed-off-by: Irui Wang <[email protected]>
---
.../platform/mediatek/vcodec/encoder/venc/venc_h264_if.c | 5 +++++
.../platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
index a68dac72c4e4..385bcc0d14f3 100644
--- a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
+++ b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
@@ -597,6 +597,11 @@ static int h264_enc_init(struct mtk_vcodec_enc_ctx *ctx)
inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base, VENC_SYS);
ret = vpu_enc_init(&inst->vpu_inst);
+ if (!inst->vpu_inst.vsi) {
+ mtk_venc_err(ctx, "share buffer is NULL");
+ kfree(inst);
+ return -EFAULT;
+ }
if (MTK_ENC_IOVA_IS_34BIT(ctx))
inst->vsi_34 = (struct venc_h264_vsi_34 *)inst->vpu_inst.vsi;
diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
index 05abca91e742..23ca0d93324f 100644
--- a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
+++ b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
@@ -326,6 +326,11 @@ static int vp8_enc_init(struct mtk_vcodec_enc_ctx *ctx)
inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base, VENC_LT_SYS);
ret = vpu_enc_init(&inst->vpu_inst);
+ if (!inst->vpu_inst.vsi) {
+ mtk_venc_err(ctx, "share buffer is NULL");
+ kfree(inst);
+ return -EFAULT;
+ }
inst->vsi = (struct venc_vp8_vsi *)inst->vpu_inst.vsi;
--
2.25.1
On 26/09/2023 12:19, Irui Wang wrote:
> Need to set the private data with encoder device, or will access
> NULL pointer in encoder handler.
>
> Fixes: 1972e32431ed ("media: mediatek: vcodec: Fix possible invalid memory access for encoder")
>
> Signed-off-by: Irui Wang <[email protected]>
> ---
> drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> index d299cc2962a5..ae6290d28f8e 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> @@ -138,7 +138,8 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
> vpu->ctx->vpu_inst = vpu;
>
> status = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu->id,
> - vpu_enc_ipi_handler, "venc", NULL);
> + vpu_enc_ipi_handler, "venc",
> + vpu->ctx->dev);
>
> if (status) {
> mtk_venc_err(vpu->ctx, "vpu_ipi_register fail %d", status);
Is this a fix that should go to 6.6?
Regards,
Hans
On 26/09/2023 12:19, Irui Wang wrote:
> There will be a kernel null pointer exception if 'vsi' is NULL, check
> 'vsi' is not NULL before assign it to encoder instance.
>
> Signed-off-by: Irui Wang <[email protected]>
I see no Fixes tag, is that correct?
Is this a fix that needs to go to kernel 6.6? It's not clear how urgent this
is.
Regards,
Hans
> ---
> .../platform/mediatek/vcodec/encoder/venc/venc_h264_if.c | 5 +++++
> .../platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c | 5 +++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> index a68dac72c4e4..385bcc0d14f3 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> @@ -597,6 +597,11 @@ static int h264_enc_init(struct mtk_vcodec_enc_ctx *ctx)
> inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base, VENC_SYS);
>
> ret = vpu_enc_init(&inst->vpu_inst);
> + if (!inst->vpu_inst.vsi) {
> + mtk_venc_err(ctx, "share buffer is NULL");
> + kfree(inst);
> + return -EFAULT;
> + }
>
> if (MTK_ENC_IOVA_IS_34BIT(ctx))
> inst->vsi_34 = (struct venc_h264_vsi_34 *)inst->vpu_inst.vsi;
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> index 05abca91e742..23ca0d93324f 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> @@ -326,6 +326,11 @@ static int vp8_enc_init(struct mtk_vcodec_enc_ctx *ctx)
> inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base, VENC_LT_SYS);
>
> ret = vpu_enc_init(&inst->vpu_inst);
> + if (!inst->vpu_inst.vsi) {
> + mtk_venc_err(ctx, "share buffer is NULL");
> + kfree(inst);
> + return -EFAULT;
> + }
>
> inst->vsi = (struct venc_vp8_vsi *)inst->vpu_inst.vsi;
>
Il 26/09/23 12:19, Irui Wang ha scritto:
> There will be a kernel null pointer exception if 'vsi' is NULL, check
> 'vsi' is not NULL before assign it to encoder instance.
>
> Signed-off-by: Irui Wang <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 26/09/23 12:19, Irui Wang ha scritto:
> Need to set the private data with encoder device, or will access
> NULL pointer in encoder handler.
>
> Fixes: 1972e32431ed ("media: mediatek: vcodec: Fix possible invalid memory access for encoder")
>
> Signed-off-by: Irui Wang <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 02/10/23 12:48, AngeloGioacchino Del Regno ha scritto:
> Il 26/09/23 12:19, Irui Wang ha scritto:
>> There will be a kernel null pointer exception if 'vsi' is NULL, check
>> 'vsi' is not NULL before assign it to encoder instance.
>>
>> Signed-off-by: Irui Wang <[email protected]>
>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
>
Sorry I just noticed that there's no Fixes tag.
This commit needs a Fixes tag, please add one and send a v2.
Ping! Is this a fix for 6.6 or not?
Regards,
Hans
On 02/10/2023 12:24, Hans Verkuil wrote:
> On 26/09/2023 12:19, Irui Wang wrote:
>> Need to set the private data with encoder device, or will access
>> NULL pointer in encoder handler.
>>
>> Fixes: 1972e32431ed ("media: mediatek: vcodec: Fix possible invalid memory access for encoder")
>>
>> Signed-off-by: Irui Wang <[email protected]>
>> ---
>> drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
>> index d299cc2962a5..ae6290d28f8e 100644
>> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
>> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
>> @@ -138,7 +138,8 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
>> vpu->ctx->vpu_inst = vpu;
>>
>> status = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu->id,
>> - vpu_enc_ipi_handler, "venc", NULL);
>> + vpu_enc_ipi_handler, "venc",
>> + vpu->ctx->dev);
>>
>> if (status) {
>> mtk_venc_err(vpu->ctx, "vpu_ipi_register fail %d", status);
>
> Is this a fix that should go to 6.6?
>
> Regards,
>
> Hans
On 04/10/2023 08:54, Hans Verkuil wrote:
> Ping! Is this a fix for 6.6 or not?
>
> Regards,
>
> Hans
>
> On 02/10/2023 12:24, Hans Verkuil wrote:
>> On 26/09/2023 12:19, Irui Wang wrote:
>>> Need to set the private data with encoder device, or will access
>>> NULL pointer in encoder handler.
>>>
>>> Fixes: 1972e32431ed ("media: mediatek: vcodec: Fix possible invalid memory access for encoder")
>>>
>>> Signed-off-by: Irui Wang <[email protected]>
>>> ---
>>> drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
>>> index d299cc2962a5..ae6290d28f8e 100644
>>> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
>>> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
>>> @@ -138,7 +138,8 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
>>> vpu->ctx->vpu_inst = vpu;
>>>
>>> status = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu->id,
>>> - vpu_enc_ipi_handler, "venc", NULL);
>>> + vpu_enc_ipi_handler, "venc",
>>> + vpu->ctx->dev);
>>>
>>> if (status) {
>>> mtk_venc_err(vpu->ctx, "vpu_ipi_register fail %d", status);
>>
>> Is this a fix that should go to 6.6?
This looks like a real bug, so I'll queue this up for 6.6.
Regards,
Hans
On 26/09/2023 12:19, Irui Wang wrote:
> There will be a kernel null pointer exception if 'vsi' is NULL, check
> 'vsi' is not NULL before assign it to encoder instance.
>
> Signed-off-by: Irui Wang <[email protected]>
> ---
> .../platform/mediatek/vcodec/encoder/venc/venc_h264_if.c | 5 +++++
> .../platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c | 5 +++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> index a68dac72c4e4..385bcc0d14f3 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> @@ -597,6 +597,11 @@ static int h264_enc_init(struct mtk_vcodec_enc_ctx *ctx)
> inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base, VENC_SYS);
>
> ret = vpu_enc_init(&inst->vpu_inst);
> + if (!inst->vpu_inst.vsi) {
> + mtk_venc_err(ctx, "share buffer is NULL");
> + kfree(inst);
> + return -EFAULT;
> + }
Shouldn't this check be done in vpu_enc_init?
It looks weird that a function can return 0, but there is still an
error that needs to be checked manually afterwards.
Regards,
Hans
>
> if (MTK_ENC_IOVA_IS_34BIT(ctx))
> inst->vsi_34 = (struct venc_h264_vsi_34 *)inst->vpu_inst.vsi;
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> index 05abca91e742..23ca0d93324f 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> @@ -326,6 +326,11 @@ static int vp8_enc_init(struct mtk_vcodec_enc_ctx *ctx)
> inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base, VENC_LT_SYS);
>
> ret = vpu_enc_init(&inst->vpu_inst);
> + if (!inst->vpu_inst.vsi) {
> + mtk_venc_err(ctx, "share buffer is NULL");
> + kfree(inst);
> + return -EFAULT;
> + }
>
> inst->vsi = (struct venc_vp8_vsi *)inst->vpu_inst.vsi;
>
Dear Hans,
Sorry for late response, we have just returned to office after
vacation. Yes, it's a fix and thank you very much for accepting it.
Best Regards
On Thu, 2023-10-05 at 10:46 +0200, Hans Verkuil wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 04/10/2023 08:54, Hans Verkuil wrote:
> > Ping! Is this a fix for 6.6 or not?
> >
> > Regards,
> >
> > Hans
> >
> > On 02/10/2023 12:24, Hans Verkuil wrote:
> >> On 26/09/2023 12:19, Irui Wang wrote:
> >>> Need to set the private data with encoder device, or will access
> >>> NULL pointer in encoder handler.
> >>>
> >>> Fixes: 1972e32431ed ("media: mediatek: vcodec: Fix possible
> invalid memory access for encoder")
> >>>
> >>> Signed-off-by: Irui Wang <[email protected]>
> >>> ---
> >>> drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c | 3
> ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git
> a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> >>> index d299cc2962a5..ae6290d28f8e 100644
> >>> ---
> a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> >>> +++
> b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> >>> @@ -138,7 +138,8 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
> >>> vpu->ctx->vpu_inst = vpu;
> >>>
> >>> status = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler,
> vpu->id,
> >>> - vpu_enc_ipi_handler, "venc", NULL);
> >>> + vpu_enc_ipi_handler, "venc",
> >>> + vpu->ctx->dev);
> >>>
> >>> if (status) {
> >>> mtk_venc_err(vpu->ctx, "vpu_ipi_register fail %d", status);
> >>
> >> Is this a fix that should go to 6.6?
>
> This looks like a real bug, so I'll queue this up for 6.6.
>
> Regards,
>
> Hans
Dear Angelo, Hans,
Thanks for your reviewing.
I will send patch v2 with Fix tag...
....
On Thu, 2023-10-05 at 10:43 +0200, Hans Verkuil wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 26/09/2023 12:19, Irui Wang wrote:
> > There will be a kernel null pointer exception if 'vsi' is NULL,
> check
> > 'vsi' is not NULL before assign it to encoder instance.
> >
> > Signed-off-by: Irui Wang <[email protected]>
> > ---
> > .../platform/mediatek/vcodec/encoder/venc/venc_h264_if.c | 5
> +++++
> > .../platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c | 5
> +++++
> > 2 files changed, 10 insertions(+)
> >
> > diff --git
> a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> > index a68dac72c4e4..385bcc0d14f3 100644
> > ---
> a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> > +++
> b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_h264_if.c
> > @@ -597,6 +597,11 @@ static int h264_enc_init(struct
> mtk_vcodec_enc_ctx *ctx)
> > inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base,
> VENC_SYS);
> >
> > ret = vpu_enc_init(&inst->vpu_inst);
> > +if (!inst->vpu_inst.vsi) {
> > +mtk_venc_err(ctx, "share buffer is NULL");
> > +kfree(inst);
> > +return -EFAULT;
> > +}
>
> Shouldn't this check be done in vpu_enc_init?
> It looks weird that a function can return 0, but there is still an
> error that needs to be checked manually afterwards.
>
> Regards,
>
> Hans
Also, this check will move into vpu_enc_init.
Best Regards
>
> >
> > if (MTK_ENC_IOVA_IS_34BIT(ctx))
> > inst->vsi_34 = (struct venc_h264_vsi_34 *)inst->vpu_inst.vsi;
> > diff --git
> a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> > index 05abca91e742..23ca0d93324f 100644
> > ---
> a/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> > +++
> b/drivers/media/platform/mediatek/vcodec/encoder/venc/venc_vp8_if.c
> > @@ -326,6 +326,11 @@ static int vp8_enc_init(struct
> mtk_vcodec_enc_ctx *ctx)
> > inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base,
> VENC_LT_SYS);
> >
> > ret = vpu_enc_init(&inst->vpu_inst);
> > +if (!inst->vpu_inst.vsi) {
> > +mtk_venc_err(ctx, "share buffer is NULL");
> > +kfree(inst);
> > +return -EFAULT;
> > +}
> >
> > inst->vsi = (struct venc_vp8_vsi *)inst->vpu_inst.vsi;
> >
>