Date: Mon, 11 Nov 2013 14:14:42 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Eitan Adler <lists@eitanadler.com> Cc: scsi@FreeBSD.org Subject: Re: mptutil, mfitil, expand_number Message-ID: <20131111134424.N854@besplex.bde.org> In-Reply-To: <CAF6rxgmp0osqW8i27ntXvQ5FSE=96auRHztqYJmiDvR1mv1sUA@mail.gmail.com> References: <CAF6rxgkCkpS_iu8VXcUnqk0TG%2BMrbzvt55-QJ-ebTUrqJv8UHQ@mail.gmail.com> <20131110161359.S1474@besplex.bde.org> <CAF6rxgmp0osqW8i27ntXvQ5FSE=96auRHztqYJmiDvR1mv1sUA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 10 Nov 2013, Eitan Adler wrote:
> On Sun, Nov 10, 2013 at 1:25 AM, Bruce Evans <brde@optusnet.com.au> 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 <sys/param.h> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131111134424.N854>