| Message ID | 7f4fe87ef8a9995bc2c64bf2e5a03ef6948b8692.camel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: staging: media: omap4iss: Fix null dereference for iss | expand |
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; > >
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
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;