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>
index | next in thread | previous in thread | raw e-mail
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
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131111134424.N854>