From owner-freebsd-scsi@FreeBSD.ORG Mon Nov 11 04:21:42 2013 Return-Path: Delivered-To: scsi@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 5C613E81 for ; Mon, 11 Nov 2013 04:21:42 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id B9BCC2709 for ; Mon, 11 Nov 2013 04:21:41 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 8AE243CDA1C; Mon, 11 Nov 2013 14:14:45 +1100 (EST) Date: Mon, 11 Nov 2013 14:14:42 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eitan Adler Subject: Re: mptutil, mfitil, expand_number In-Reply-To: Message-ID: <20131111134424.N854@besplex.bde.org> References: <20131110161359.S1474@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=bpB1Wiqi c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=WvzHK2nvTcUA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=oiRXuiCKKlcA:10 a=wr-8jAivI19nOVbf5igA:9 a=8xKpZ07xwyGC5BvV:21 a=ygRQkgweRc6PhHQw:21 a=CjuIK1q_8ugA:10 Cc: scsi@FreeBSD.org X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Nov 2013 04:21:42 -0000 On Sun, 10 Nov 2013, Eitan Adler wrote: > On Sun, Nov 10, 2013 at 1:25 AM, Bruce Evans wrote: > Lets see if I understand everything you wanted me to do. I am not > intending to fix any bugs in expand_number at the moment if any. > diff --git a/usr.sbin/mptutil/mpt_config.c b/usr.sbin/mptutil/mpt_config.c > index 17b9945..0d34d16 100644 > --- a/usr.sbin/mptutil/mpt_config.c > +++ b/usr.sbin/mptutil/mpt_config.c > ... > @@ -615,7 +586,7 @@ create_volume(int ac, char **av) > CONFIG_PAGE_RAID_VOL_0 *vol; > struct config_id_state state; > struct volume_info *info; > - long stripe_size; > + uint64_t stripe_size; > int ch, error, fd, i, quick, raid_type, verbose; > #ifdef DEBUG > int dump; Blindly changing the type is not clearly safe. In fact, it is not safe. After initialization, it is only used to pass to functions expecting a long, and there is a prototype that blindly converts it to long. Overflow can then occur on 32-bit systems, since you didn't fix the range checking. > @@ -666,7 +637,11 @@ create_volume(int ac, char **av) > quick = 1; > break; > case 's': > - stripe_size = dehumanize(optarg); > + error = expand_number(optarg, &stripe_size); > + if (error == -1) { > + warnx("Invalid stripe size %s", optarg); > + return (errno); > + } > if ((stripe_size < 512) || (!powerof2(stripe_size))) { > warnx("Invalid stripe size %s", optarg); > return (EINVAL); Overflow occurs when stripe_size is used when it exceeds > LONG_MAX. This can give a negative number. E.g., on 32-bit 2's complement systems, stripe_size might be 0x80000000 here. This gets converted to LONG_MIN. Dividing this by 512 gives -4194304 in vol->StripeSize. The previous overflow bugs could never give a negative value, since if dehumanize() overflows to a negative value then in the above it doesn't pass the !(stripe_size < 512) test. Larger values can overflow to anything. This should be fixed by checking the range from above like I said before. I forgot to check before if powerof2() works. It was invalid to call it with a long arg, and the type change fixes this. powerof2() and nearby functions in require an unsigned type, or a strictly positive (nonzero) signed value, or pure 2's complement. A range check from above would have fixed this too. Of course, systemish code like this assumes pure 2's complement for everthing. Bruce