s3c-camif: array underflow in __camif_subdev_try_format()

PATCH In review Dan Carpenter 3 participants 4 messages Jan 22, 2018 – Jan 24, 2018

Subsystems (3)

Media (V4L/DVB) Kernel janitors Arm Samsung Exynos

lore

Share & tools

lore

Dan Carpenter

author

2018-01-22 10:37 UTC #0 lore raw

PATCH 1/1 s3c-camif: array underflow in __camif_subdev_try_format()

The while loop is a post op, "while (i-- >= 0)" so the last iteration will read camif_mbus_formats[-1] and then the loop will exit with "i" set to -2 and so we do: "mf->code = camif_mbus_formats[-2];".

I've changed it to a pre-op, I've added a check to ensure we found the right format and I've removed the "mf->code = camif_mbus_formats[i];" because that is a no-op anyway.

Fixes babde1c243b2 ("[media] V4L: Add driver for S3C24XX/S3C64XX SoC series camera interface")

Signed-off-by Dan Carpenter <[email protected]>

drivers/media/platform/s3c-camif/camif-capture.c +3 -3

index 437395a61065..012f4b389c55 100644
@@ -1261,11 +1261,11 @@ static void __camif_subdev_try_format(struct camif_dev *camif,
 	/* FIXME: constraints against codec or preview path ? */
 	pix_lim = &variant->vp_pix_limits[VP_CODEC];
 
-	while (i-- >= 0)
+	while (--i >= 0)
 		if (camif_mbus_formats[i] = mf->code)
 			break;
-
-	mf->code = camif_mbus_formats[i];
+	if (i < 0)
+		return;
 
 	if (pad = CAMIF_SD_PAD_SINK) {
 		v4l_bound_align_image(&mf->width, 8, CAMIF_MAX_PIX_WIDTH,

Sylwester Nawrocki

2018-01-22 20:50 UTC #1 lore raw

Quoted 17 lines · Dan Carpenter

The while loop is a post op, "while (i-- >= 0)" so the last iteration will read camif_mbus_formats[-1] and then the loop will exit with "i" set to -2 and so we do: "mf->code = camif_mbus_formats[-2];".
I've changed it to a pre-op, I've added a check to ensure we found the right format and I've removed the "mf->code = camif_mbus_formats[i];" because that is a no-op anyway.
Fixes babde1c243b2 ("[media] V4L: Add driver for S3C24XX/S3C64XX SoC series camera interface")
Signed-off-by Dan Carpenter <[email protected]>
drivers/media/platform/s3c-camif/camif-capture.c +0 -0
index 437395a61065..012f4b389c55 100644
@@ -1261,11 +1261,11 @@ static void __camif_subdev_try_format(struct camif_dev *camif,
  	/* FIXME: constraints against codec or preview path ? */

drivers/media/platform/s3c-camif/camif-capture.c static void __camif_subdev_try_format(struct camif_dev *camif, · line 1268

Quoted 10 lines

drivers/media/platform/s3c-camif/camif-capture.c +3 -3

  	pix_lim = &variant->vp_pix_limits[VP_CODEC];
 
-	while (i-- >= 0)
+	while (--i >= 0)
  		if (camif_mbus_formats[i] = mf->code)
  			break;
-
-	mf->code = camif_mbus_formats[i];
+	if (i < 0)
+		return;

Thanks for the patch Dan. mf->width needs to be aligned by this try_format function so we shouldn't return here. Also it needs to be ensured mf->code is set to one of the supported values when this function returns. Sorry, the current code really doesn't give a clue what was intended.

There is already queued a patch from Arnd [1] addressing the issues you have found.

drivers/media/platform/s3c-camif/camif-capture.c static void __camif_subdev_try_format(struct camif_dev *camif,

Quoted 3 lines

drivers/media/platform/s3c-camif/camif-capture.c +0 -0

  	if (pad = CAMIF_SD_PAD_SINK) {
  		v4l_bound_align_image(&mf->width, 8, CAMIF_MAX_PIX_WIDTH,

[1] https://patchwork.linuxtv.org/patch/46508

--
Regards,
Sylwester

Dan Carpenter

author

2018-01-24 08:13 UTC #2 lore raw

Quoted 21 lines · Sylwester Nawrocki

Quoted 4 lines · Dan Carpenter
drivers/media/platform/s3c-camif/camif-capture.c +0 -0
@@ -1261,11 +1261,11 @@ static void __camif_subdev_try_format(struct camif_dev *camif,
  	/* FIXME: constraints against codec or preview path ? */
drivers/media/platform/s3c-camif/camif-capture.c static void __camif_subdev_try_format(struct camif_dev *camif, · line 1268
Quoted 10 lines
drivers/media/platform/s3c-camif/camif-capture.c +3 -3
  	pix_lim = &variant->vp_pix_limits[VP_CODEC];
 
-	while (i-- >= 0)
+	while (--i >= 0)
  		if (camif_mbus_formats[i] = mf->code)
  			break;
-
-	mf->code = camif_mbus_formats[i];
+	if (i < 0)
+		return;
Thanks for the patch Dan. mf->width needs to be aligned by this try_format function so we shouldn't return here. Also it needs to be ensured mf->code is set to one of the supported values when this function returns. Sorry, the current code really doesn't give a clue what was intended.

Quoted 9 lines

There is already queued a patch from Arnd [1] addressing the issues you have found.
drivers/media/platform/s3c-camif/camif-capture.c static void __camif_subdev_try_format(struct camif_dev *camif,
Quoted 3 lines
drivers/media/platform/s3c-camif/camif-capture.c +0 -0
  	if (pad = CAMIF_SD_PAD_SINK) {
  		v4l_bound_align_image(&mf->width, 8, CAMIF_MAX_PIX_WIDTH,
 
[1] https://patchwork.linuxtv.org/patch/46508

Hey Arnd,

I happened to be looking at the same bugs but using Smatch. Did you get these two bugs as well?

drivers/scsi/sym53c8xx_2/sym_hipd.c:549 sym_getsync() error: iterator underflow 'div_10M' (-1)-255 drivers/media/i2c/sr030pc30.c:522 try_fmt() error: iterator underflow 'sr030pc30_formats' (-1)-4

regards,
dan carpenter

Arnd Bergmann

2018-01-24 08:43 UTC #3 lore raw

Quoted 8 lines · Dan Carpenter

Quoted 1 line · Sylwester Nawrocki
On 01/22/2018 11:37 AM, Dan Carpenter wrote:
I happened to be looking at the same bugs but using Smatch. Did you get these two bugs as well?
drivers/scsi/sym53c8xx_2/sym_hipd.c:549 sym_getsync() error: iterator underflow 'div_10M' (-1)-255 drivers/media/i2c/sr030pc30.c:522 try_fmt() error: iterator underflow 'sr030pc30_formats' (-1)-4

I don't recall seeing those two, here is a list of array-bounds warnings I had in the past
months, mostly while building with gcc-7.2:

/git/arm-soc/arch/arm/mach-vexpress/spc.c:431:38: warning: array subscript is below array bounds [-Warray-bounds] /git/arm-soc/arch/x86/include/asm/string_32.h:70:16: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/arch/x86/include/asm/uaccess_64.h:143:20: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/drivers/cpufreq/arm_big_little.c:201:24: warning: array subscript is above array bounds [-Warray-bounds] /git/arm-soc/drivers/cpufreq/arm_big_little.c:325:16: warning: array subscript is above array bounds [-Warray-bounds] /git/arm-soc/drivers/cpufreq/arm_big_little.c:440:39: warning: array subscript is above array bounds [-Warray-bounds] /git/arm-soc/drivers/dma/sh/rcar-dmac.c:876:29: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/drivers/isdn/hardware/eicon/message.c:11302:54: error:
array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/drivers/net/ethernet/intel/igb/igb_ptp.c:367: error:
array subscript is below array bounds /git/arm-soc/drivers/net/ethernet/intel/igb/igb_ptp.c:455: error:
array subscript is below array bounds /git/arm-soc/drivers/scsi/qla2xxx/qla_gs.c:1398:7: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/drivers/scsi/qla2xxx/qla_gs.c:2279:7: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/drivers/scsi/sym53c416.c:565:58: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c:492:14:
error: array subscript -1 is below array bounds of 'struct ptlrpcd_ctl[]' [-Werror=array-bounds] /git/arm-soc/fs/f2fs/segment.c:3044:5: error: array subscript is below array bounds [-Werror=array-bounds] /git/arm-soc/include/linux/compiler.h:253:20: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/include/linux/dynamic_debug.h:86:20: warning: array subscript is above array bounds [-Warray-bounds] /git/arm-soc/include/sound/pcm.h:919:9: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/kernel/bpf/verifier.c:4320:29: error: array subscript is above array bounds [-Werror=array-bounds] /git/arm-soc/kernel/rcu/tree.c:3332:13: warning: array subscript is above array bounds [-Warray-bounds] /git/arm-soc/net/ipv4/tcp_output.c:2129:40: error: array subscript is below array bounds [-Werror=array-bounds] /git/arm-soc/net/ipv4/tcp_output.c:2207:40: error: array subscript is below array bounds [-Werror=array-bounds] /git/arm-soc/net/rxrpc/ar-connection.c:589:16: warning: array subscript is above array bounds [-Warray-bounds] /git/arm-soc/sound/soc/sh/rcar/cmd.c:88:14: error: array subscript is below array bounds [-Werror=array-bounds] /git/arm-soc/sound/soc/sh/rcar/cmd.c:88: error: array subscript is below array bounds

I've also opened two gcc bugs for warnings that appeared to be issued in error:

https://gcc.gnu.org/bugzilla/show_bug.cgi?idƒ312
https://gcc.gnu.org/bugzilla/show_bug.cgi?id�601

I haven't built with gcc-8 in a while, should try that again now that PR83312 has been
marked 'fixed'.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html