Re: [PATCH] media: rkisp1: rsz: crash fix when setting src format

From: Sebastian Fricke <sebastian.fricke.linux@gmail.com>
To: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: linux-media@vger.kernel.org, laurent.pinchart@ideasonboard.com,
	helen.koike@collabora.com, ezequiel@collabora.com,
	hverkuil@xs4all.nl, kernel@collabora.com, dafna3@gmail.com,
	sakari.ailus@linux.intel.com, linux-rockchip@lists.infradead.org,
	mchehab@kernel.org, tfiga@chromium.org
Subject: Re: [PATCH] media: rkisp1: rsz: crash fix when setting src format
Date: Wed, 9 Dec 2020 20:13:07 +0100	[thread overview]
Message-ID: <20201209191307.smr27xutbizjp56w@basti.Speedport_W_724V_Typ_A_05011603_06_001> (raw)
In-Reply-To: <20201209161510.31399-1-dafna.hirschfeld@collabora.com>

Hey Dafna,

Thanks for the patch, I quickly tested it and did some proofreading.

I was able to recreate the crash with following commands:

"media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":1 [fmt:YUYV8_1_5X8/900x800]'
"media-ctl" "-d" "platform:rkisp1" "--set-v4l2" '"rkisp1_resizer_mainpath":1 [fmt:YUYV8_2X8/900x800]'

# This causes the crash:
"Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004"

This patch fixed the problem.

On 09.12.2020 17:15, Dafna Hirschfeld wrote:
>When setting the source media bus code in the resizer,
>we first check that the current media bus code in the
>source is yuv encoded format. This is done by

s/is/is a/

>retrieving the data from the formats list of the isp
>entity. This cause a crash when the media bus code on the

s/cause/causes/

>source is YUYV8_1_5X8 which is not supported by the isp
>entity. Instead we should test the sink format of the resizer
>which is guaranteed to be supported by the isp entity.
>
>Fixes: 251b6eebb6c49 ("media: staging: rkisp1: rsz: Add support to more YUV encoded mbus codes on src pad")
>Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Tested-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com>

>---
> drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>index 813670ed9577..79deed8adcea 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
>@@ -520,14 +520,15 @@ static void rkisp1_rsz_set_src_fmt(struct rkisp1_resizer *rsz,
> 				   struct v4l2_mbus_framefmt *format,
> 				   unsigned int which)
> {
>-	const struct rkisp1_isp_mbus_info *mbus_info;
>-	struct v4l2_mbus_framefmt *src_fmt;
>+	const struct rkisp1_isp_mbus_info *sink_mbus_info;
>+	struct v4l2_mbus_framefmt *src_fmt, *sink_fmt;
>
>+	sink_fmt = rkisp1_rsz_get_pad_fmt(rsz, cfg, RKISP1_RSZ_PAD_SINK, which);
> 	src_fmt = rkisp1_rsz_get_pad_fmt(rsz, cfg, RKISP1_RSZ_PAD_SRC, which);
>-	mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code);
>+	sink_mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
>
> 	/* for YUV formats, userspace can change the mbus code on the src pad if it is supported */
>-	if (mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV &&
>+	if (sink_mbus_info->pixel_enc == V4L2_PIXEL_ENC_YUV &&
> 	    rkisp1_rsz_get_yuv_mbus_info(format->code))
> 		src_fmt->code = format->code;
>
>-- 
>2.17.1
>