mptutil, mfitil, expand_number

[Bruce Evans]

On Sun, 10 Nov 2013, Eitan Adler wrote:

> On Sun, Nov 10, 2013 at 1:25 AM, Bruce Evans <brde at 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