[PATCH] media: staging: media: omap4iss: Fix null dereference for iss

[PATCH] media: staging: media: omap4iss: Fix null dereference for iss

[Tanmay Bhushan]

From 7aa39c0d02bddf9cfa14762f115303b79bfa0ae3 Mon Sep 17 00:00:00 2001
From: Tanmay Bhushan <007047221b@gmail.com>
Date: Wed, 28 Dec 2022 21:01:16 +0100
Subject: [PATCH] media: staging: media: omap4iss: Fix null dereference
for iss

media_pad_remote_pad_first returns NULL in some cases but while using
the return value was used without NULL check which will lead to panic
in case of NULL return. iss_pipeline_is_last returns value check so
have returned 0 in case of NULL and csi2_configure is not documented
for such cases so returned EINVAL for it. Code is not tested
as it is only for NULL dereference verification.

Signed-off-by: Tanmay Bhushan <007047221b@gmail.com>
---
 drivers/staging/media/omap4iss/iss.c      | 6 +++++-
 drivers/staging/media/omap4iss/iss_csi2.c | 4 ++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/omap4iss/iss.c
b/drivers/staging/media/omap4iss/iss.c
index fa2a36d829d3..3f01eeff40e7 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -552,7 +552,11 @@ static int iss_pipeline_is_last(struct
media_entity *me)
 	if (!pipe || pipe->stream_state ==
ISS_PIPELINE_STREAM_STOPPED)
 		return 0;
 	pad = media_pad_remote_pad_first(&pipe->output->pad);
-	return pad->entity == me;
+
+	if (pad)
+		return pad->entity == me;
+
+	return 0;
 }
 
 static int iss_reset(struct iss_device *iss)
diff --git a/drivers/staging/media/omap4iss/iss_csi2.c
b/drivers/staging/media/omap4iss/iss_csi2.c
index 04ce0e7eb557..ab2c2ad64464 100644
--- a/drivers/staging/media/omap4iss/iss_csi2.c
+++ b/drivers/staging/media/omap4iss/iss_csi2.c
@@ -539,6 +539,10 @@ static int csi2_configure(struct iss_csi2_device
*csi2)
 		return -EBUSY;
 
 	pad = media_pad_remote_pad_first(&csi2->pads[CSI2_PAD_SINK]);
+
+	if (!pad)
+		return -EINVAL;
+
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	pdata = sensor->host_priv;
 
-- 
2.34.1

Re: [PATCH] media: staging: media: omap4iss: Fix null dereference for iss

[Laurent Pinchart]

Hi Tanmay,

Thank you for the patch.

On Wed, Dec 28, 2022 at 09:58:31PM +0100, Tanmay Bhushan wrote:
> From 7aa39c0d02bddf9cfa14762f115303b79bfa0ae3 Mon Sep 17 00:00:00 2001
> From: Tanmay Bhushan <007047221b@gmail.com>
> Date: Wed, 28 Dec 2022 21:01:16 +0100
> Subject: [PATCH] media: staging: media: omap4iss: Fix null dereference
> for iss
> 
> media_pad_remote_pad_first returns NULL in some cases but while using
> the return value was used without NULL check which will lead to panic
> in case of NULL return. iss_pipeline_is_last returns value check so
> have returned 0 in case of NULL and csi2_configure is not documented
> for such cases so returned EINVAL for it. Code is not tested
> as it is only for NULL dereference verification.
> 
> Signed-off-by: Tanmay Bhushan <007047221b@gmail.com>
> ---
>  drivers/staging/media/omap4iss/iss.c      | 6 +++++-
>  drivers/staging/media/omap4iss/iss_csi2.c | 4 ++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c
> b/drivers/staging/media/omap4iss/iss.c
> index fa2a36d829d3..3f01eeff40e7 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -552,7 +552,11 @@ static int iss_pipeline_is_last(struct
> media_entity *me)

Your mail client wrapped lines, which prevents the patch from being
applied with git-am. I recommend using git-send-email to send patches.
https://git-send-email.io/ provides clear and detailed instructions on
how to set it up (especially when using gmail).

>  	if (!pipe || pipe->stream_state ==
> ISS_PIPELINE_STREAM_STOPPED)
>  		return 0;
>  	pad = media_pad_remote_pad_first(&pipe->output->pad);
> -	return pad->entity == me;

Have you seen this actually crashing, or are you only speculating ? The
video node at the output of the pipeline should always be connected, so
I don't think media_pad_remote_pad_first() can ever return NULL here.

> +
> +	if (pad)
> +		return pad->entity == me;
> +
> +	return 0;
>  }
>  
>  static int iss_reset(struct iss_device *iss)
> diff --git a/drivers/staging/media/omap4iss/iss_csi2.c
> b/drivers/staging/media/omap4iss/iss_csi2.c
> index 04ce0e7eb557..ab2c2ad64464 100644
> --- a/drivers/staging/media/omap4iss/iss_csi2.c
> +++ b/drivers/staging/media/omap4iss/iss_csi2.c
> @@ -539,6 +539,10 @@ static int csi2_configure(struct iss_csi2_device
> *csi2)
>  		return -EBUSY;
>  
>  	pad = media_pad_remote_pad_first(&csi2->pads[CSI2_PAD_SINK]);
> +
> +	if (!pad)
> +		return -EINVAL;

Same here, what makes you think this is possible ?

> +
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
>  	pdata = sensor->host_priv;
>  
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: staging: media: omap4iss: Fix null dereference for iss

[Greg Kroah-Hartman]

On Wed, Dec 28, 2022 at 11:19:54PM +0100, Tanmay wrote:
> Hi Laurent,
> 
> Thank you. I will take care of the wrapped lines. I haven't personally seen
> it return Null so yes
> in the best scenario it is a speculation but in the worst scenario it
> shouldn't hurt.

We do not add checks for when things are "impossible" to hit, otherwise
the kernel would be full of unneeded and useless checks everywhere.
Only test for things that can actually happen please.

thanks,

greg k-h