[PATCH 1/2] media: mediatek: vcodec: Fix encoder access NULL pointer

[PATCH 1/2] media: mediatek: vcodec: Fix encoder access NULL pointer

[Irui Wang]

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 <irui.wang@mediatek.com>
---
 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

[PATCH 2/2] media: mediatek: vcodec: Handle encoder vsi NULL pointer case

[Irui Wang]

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 <irui.wang@mediatek.com>
---
 .../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

Re: [PATCH 1/2] media: mediatek: vcodec: Fix encoder access NULL pointer

[Hans Verkuil]

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 <irui.wang@mediatek.com>
> ---
>  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

Re: [PATCH 2/2] media: mediatek: vcodec: Handle encoder vsi NULL pointer case

[Hans Verkuil]

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 <irui.wang@mediatek.com>

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;
>  

Re: [PATCH 2/2] media: mediatek: vcodec: Handle encoder vsi NULL pointer case

[AngeloGioacchino Del Regno]

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 <irui.wang@mediatek.com>

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

Re: [PATCH 1/2] media: mediatek: vcodec: Fix encoder access NULL pointer

[AngeloGioacchino Del Regno]

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 <irui.wang@mediatek.com>

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

Re: [PATCH 2/2] media: mediatek: vcodec: Handle encoder vsi NULL pointer case

[AngeloGioacchino Del Regno]

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 <irui.wang@mediatek.com>
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 

Sorry I just noticed that there's no Fixes tag.

This commit needs a Fixes tag, please add one and send a v2.

Re: [PATCH 1/2] media: mediatek: vcodec: Fix encoder access NULL pointer

[Hans Verkuil]

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 <irui.wang@mediatek.com>
>> ---
>>  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

Re: [PATCH 2/2] media: mediatek: vcodec: Handle encoder vsi NULL pointer case

[Hans Verkuil]

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 <irui.wang@mediatek.com>
> ---
>  .../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;
>  

Re: [PATCH 1/2] media: mediatek: vcodec: Fix encoder access NULL pointer

[Hans Verkuil]

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 <irui.wang@mediatek.com>
>>> ---
>>>  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

Re: [PATCH 1/2] media: mediatek: vcodec: Fix encoder access NULL pointer

[Irui Wang (王瑞)]

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 <irui.wang@mediatek.com>
> >>> ---
> >>>  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

Re: [PATCH 2/2] media: mediatek: vcodec: Handle encoder vsi NULL pointer case

[Irui Wang (王瑞)]

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 <irui.wang@mediatek.com>
> > ---
> >  .../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;
> >  
>