Skip to contentSkip to conversation

media: staging: media: omap4iss: Fix null dereference for iss

PATCH In review Tanmay Bhushan 3 participants 3 messages Dec 28, 2022 – Dec 29, 2022 1 message not in the archive

Subsystems (2)
Media (V4L/DVB) Staging
lore
Share & tools
lore

Tanmay Bhushan

author
#0 lore raw

PATCH 1/1 media: staging: media: omap4iss: Fix null dereference for iss

From 7aa39c0d02bddf9cfa14762f115303b79bfa0ae3 Mon Sep 17 00:00:00 2001
From: Tanmay Bhushan <[email protected]>
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 <[email protected]>
---
 drivers/staging/media/omap4iss/iss.c      | 6 +++++-
 drivers/staging/media/omap4iss/iss_csi2.c | 4 ++++
 2 files changed, 9 insertions(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/staging/media/omap4iss/iss.c
index fa2a36d829d3..3f01eeff40e7 100644

drivers/staging/media/omap4iss/iss.c +0 -0
@@ -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)

(quoted diff) +0 -0

b/drivers/staging/media/omap4iss/iss_csi2.c
index 04ce0e7eb557..ab2c2ad64464 100644

drivers/staging/media/omap4iss/iss_csi2.c +0 -0
@@ -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;

Signature · 1 line
2.34.1
#1 lore raw

Hi Tanmay,

Thank you for the patch.

Quoted 25 lines · Tanmay Bhushan

From 7aa39c0d02bddf9cfa14762f115303b79bfa0ae3 Mon Sep 17 00:00:00 2001
From: Tanmay Bhushan <[email protected]>
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 <[email protected]>
---
 drivers/staging/media/omap4iss/iss.c      | 6 +++++-
 drivers/staging/media/omap4iss/iss_csi2.c | 4 ++++
 2 files changed, 9 insertions(+), 1 deletion(-)
drivers/staging/media/omap4iss/iss.c +0 -0
index fa2a36d829d3..3f01eeff40e7 100644

drivers/staging/media/omap4iss/iss.c static int iss_pipeline_is_last(struct · line 552

Quoted 1 line
drivers/staging/media/omap4iss/iss.c +0 -0
@@ -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).

drivers/staging/media/omap4iss/iss.c static int iss_pipeline_is_last(struct · line 557

Quoted 5 lines
drivers/staging/media/omap4iss/iss.c +0 -1
 	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.

Quoted 14 lines
drivers/staging/media/omap4iss/iss.c +5 -0
+
+	if (pad)
+		return pad->entity == me;
+
+	return 0;
 }

static int iss_reset(struct iss_device *iss)

drivers/staging/media/omap4iss/iss_csi2.c +0 -0
index 04ce0e7eb557..ab2c2ad64464 100644

drivers/staging/media/omap4iss/iss_csi2.c static int csi2_configure(struct iss_csi2_device · line 545

Quoted 7 lines
drivers/staging/media/omap4iss/iss_csi2.c +3 -0
@@ -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 ?

Quoted 5 lines
drivers/staging/media/omap4iss/iss_csi2.c +1 -0
+
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	pdata = sensor->host_priv;
Signature · 3 lines
Regards,

Laurent Pinchart

A message that is not in the archive (sent off-list or not yet received): CAKKF0qA8oB7ZkkQ5-bj=66sP+WmjL6gUDV5EMTBM3SSW5_+qXA@mail.gmail.com

maintainer
#3 lore raw
Quoted 6 lines · Tanmay

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