Message ID | 20180122103714.GA25044@mwanda (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1edZTl-0001Hy-Ss; Mon, 22 Jan 2018 10:37:30 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751143AbeAVKh2 (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 22 Jan 2018 05:37:28 -0500 Received: from aserp2130.oracle.com ([141.146.126.79]:42038 "EHLO aserp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751093AbeAVKh0 (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 22 Jan 2018 05:37:26 -0500 Received: from pps.filterd (aserp2130.oracle.com [127.0.0.1]) by aserp2130.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w0MAb6HZ018935; Mon, 22 Jan 2018 10:37:23 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : mime-version : content-type; s=corp-2017-10-26; bh=dblmvBNA9suD6xW63LlcAwCQ1sZoTrbJSheIHeAQ70I=; b=b35DvGe80Oeb9COYDxm7Dz8cr6xT/cmBDk96WcNXN31mcL8L9zicaf+9r2YkYjpyHru1 Tx6O+Fq3RyCwPquArpTg8wlElJyNZlHQspZl0263u7crGfjAHuhQZo5sVd7Mqu5tOTVo 4zfFXFmqISQpAUuWlNkeYLN3J3ejGfQXbluBHUmTNG+Li5cT2HJp1ayP+DBOVbGy73PD NR6Q4Lb6L1WOxHFt4V7zJKwc+gGZnwlTNUEs4vlXAWr4lMct9lqoGM2tUHnLyL/A2v1r 7RG17CvBk/LrS3WEgJXGeh0ic2qSQTRC0CHNx0nnL0ElxWdqylDS2z8KIxzBoI9v3BNK wQ== Received: from aserv0021.oracle.com (aserv0021.oracle.com [141.146.126.233]) by aserp2130.oracle.com with ESMTP id 2fne8s814a-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 22 Jan 2018 10:37:23 +0000 Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserv0021.oracle.com (8.14.4/8.14.4) with ESMTP id w0MAbML7016933 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 22 Jan 2018 10:37:22 GMT Received: from abhmp0005.oracle.com (abhmp0005.oracle.com [141.146.116.11]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id w0MAbL2K014297; Mon, 22 Jan 2018 10:37:22 GMT Received: from mwanda (/41.202.241.56) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 22 Jan 2018 02:37:21 -0800 Date: Mon, 22 Jan 2018 13:37:14 +0300 From: Dan Carpenter <dan.carpenter@oracle.com> To: Sylwester Nawrocki <sylvester.nawrocki@gmail.com> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, linux-samsung-soc@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: [PATCH] [media] s3c-camif: array underflow in __camif_subdev_try_format() Message-ID: <20180122103714.GA25044@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Mailer: git-send-email haha only kidding User-Agent: Mutt/1.9.2 (2017-12-15) X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8781 signatures=668655 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=883 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1801220153 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Dan Carpenter
Jan. 22, 2018, 10:37 a.m. UTC
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 <dan.carpenter@oracle.com>
Comments
On 01/22/2018 11:37 AM, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> > > diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c > index 437395a61065..012f4b389c55 100644 > --- a/drivers/media/platform/s3c-camif/camif-capture.c > +++ b/drivers/media/platform/s3c-camif/camif-capture.c > @@ -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; 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. > 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
On Mon, Jan 22, 2018 at 09:50:04PM +0100, Sylwester Nawrocki wrote: > On 01/22/2018 11:37 AM, Dan Carpenter wrote: > > --- a/drivers/media/platform/s3c-camif/camif-capture.c > > +++ b/drivers/media/platform/s3c-camif/camif-capture.c > > @@ -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; > > 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. > > > 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
On Wed, Jan 24, 2018 at 9:13 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Mon, Jan 22, 2018 at 09:50:04PM +0100, Sylwester Nawrocki wrote: >> 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=83312 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81601 I haven't built with gcc-8 in a while, should try that again now that PR83312 has been marked 'fixed'. Arnd
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c index 437395a61065..012f4b389c55 100644 --- a/drivers/media/platform/s3c-camif/camif-capture.c +++ b/drivers/media/platform/s3c-camif/camif-capture.c @@ -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,