[PATCH] media: verisilicon: Fix null pointer dereference in try_fmt

[Hans Verkuil]

On 25/05/2023 16:38, Nicolas Dufresne wrote:
> Le mardi 16 mai 2023 à 11:12 +0200, Michael Tretter a écrit :
>> Since commit db6f68b51e5c ("media: verisilicon: Do not set context
>> src/dst formats in reset functions"), vpu_src_fmt is not set in the
>> reset function, but only set in hantro_set_fmt_out, which calls
>> hantro_try_fmt before setting the format. Therefore, hantro_try_fmt
>> might be called with vpu_src_fmt still being null.
>>
>> Add a test if the format is actually set before checking the format.
>>
>> Signed-off-by: Michael Tretter <m.tretter at pengutronix.de>
>> Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats in reset functions")
> 
> This patch highlights yet more issues in the driver default format handling, but
> the remaining bug is extremely minor (too small sizeimage before S_FMT is
> called, rather then kernel oops.). Considering how long this has been going,
> please consider merging this.

I went with this fix:

https://patchwork.linuxtv.org/project/linux-media/patch/20230523162515.993862-1-benjamin.gaignard@collabora.com/

Part of this pull request:

https://patchwork.linuxtv.org/project/linux-media/patch/d4b08420-f7c0-4950-2d20-385d98f3cad9@xs4all.nl/

If you disagree, then please let me know.

This particular patch has been marked as Superseded in patchwork.

Regards,

	Hans

> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> 
>> ---
>>  drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> index 835518534e3b..ec37d2646fde 100644
>> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
>> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
>> @@ -313,17 +313,20 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
>>  		/* Fill remaining fields */
>>  		v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width,
>>  				    pix_mp->height);
>> -		if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE &&
>> +		if (ctx->vpu_src_fmt &&
>> +		    ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE &&
>>  		    !hantro_needs_postproc(ctx, fmt))
>>  			pix_mp->plane_fmt[0].sizeimage +=
>>  				hantro_h264_mv_size(pix_mp->width,
>>  						    pix_mp->height);
>> -		else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME &&
>> +		else if (ctx->vpu_src_fmt &&
>> +			 ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME &&
>>  			 !hantro_needs_postproc(ctx, fmt))
>>  			pix_mp->plane_fmt[0].sizeimage +=
>>  				hantro_vp9_mv_size(pix_mp->width,
>>  						   pix_mp->height);
>> -		else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE &&
>> +		else if (ctx->vpu_src_fmt &&
>> +			 ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE &&
>>  			 !hantro_needs_postproc(ctx, fmt))
>>  			pix_mp->plane_fmt[0].sizeimage +=
>>  				hantro_hevc_mv_size(pix_mp->width,
>