diff mbox series

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

Message ID 20230926101909.15030-1-irui.wang@mediatek.com
State Accepted
Commit 1146bec0ca34375f963f79f5c4e6b49ed5386aaa
Headers show
Series [1/2] media: mediatek: vcodec: Fix encoder access NULL pointer | expand

Commit Message

Irui Wang Sept. 26, 2023, 10:19 a.m. UTC
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(-)

Comments

Hans Verkuil Oct. 4, 2023, 6:54 a.m. UTC | #1
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
Hans Verkuil Oct. 5, 2023, 8:43 a.m. UTC | #2
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;
>
Hans Verkuil Oct. 5, 2023, 8:46 a.m. UTC | #3
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
Irui Wang Oct. 7, 2023, 1:49 a.m. UTC | #4
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
Irui Wang Oct. 7, 2023, 6:16 a.m. UTC | #5
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;
> >  
>
diff mbox series

Patch

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