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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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