Re: [Xen-devel] [PATCH v2 39/52] xen: check parameter validity when parsing command line

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.17 at 09:08, <jgross@xxxxxxxx> wrote:
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -23,9 +23,11 @@ enum system_state system_state = SYS_STATE_early_boot;
>  xen_commandline_t saved_cmdline;
>  static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
>  
> -static void __init assign_integer_param(
> +static int __init assign_integer_param(
>      const struct kernel_param *param, uint64_t val)
>  {
> +    unsigned int bits = param->len * BITS_PER_BYTE;
> +
>      switch ( param->len )
>      {
>      case sizeof(uint8_t):
> @@ -43,14 +45,17 @@ static void __init assign_integer_param(
>      default:
>          BUG();
>      }
> +
> +    return ( (val & (~0ULL << bits)) && ~(val | (~0ULL >> (65 - bits))) ) ?

The left part has undefined behavior when param->len == 8
(and on x86 I'd expect it to produce just "val"). The right part
I guess is meant to be a sign check, but that's rather obscure.
As iirc it is signed-to-unsigned conversion which has uniformly
defined behavior it may end up being better for the parameter
to be of signed type and to allow values in the range
[<type>_MIN,U<type>_MAX]. Anything more precise would
require signedness to be communicated from the *_param()
users.

Also - stray blanks inside the outermost parentheses.

And finally, wouldn't it be better to check for overflow _before_
assigning to *param->var?

> @@ -97,8 +102,9 @@ static void __init _cmdline_parse(const char *cmdline)
>                       !strncmp(param->name, opt, q + 1 - opt) )
>                  {
>                      optval[-1] = '=';
> -                    ((void (*)(const char *))param->var)(q);
> +                    rc = ((int (*)(const char *))param->var)(q);

Neither here nor in the earlier "let custom parameter parsing
routines return errno" nor in the overview you mention why this
is safe - it is not a given that caller and callee disagreeing on
return type is going to work. Just think of functions returning
aggregates or (on ix86) ones returning floating point values in
st(0).

>                      optval[-1] = '\0';
> +                    break;

Why? Applies to further break-s you add: At least in the past we
had command line options with two handlers, where each of them
needed to be invoked. I don't think we should make such impossible
even if right now there aren't any such examples. Yet if you really
mean to, then the behavioral change needs to be called out in the
description.

> @@ -106,24 +112,34 @@ static void __init _cmdline_parse(const char *cmdline)
>              switch ( param->type )
>              {
>              case OPT_STR:
> +                rc = 0;
>                  strlcpy(param->var, optval, param->len);
>                  break;
>              case OPT_UINT:
> -                assign_integer_param(
> +                rc = assign_integer_param(
>                      param,
> -                    simple_strtoll(optval, NULL, 0));
> +                    simple_strtoll(optval, &s, 0));
> +                if ( *s )
> +                    rc = -EINVAL;
>                  break;
>              case OPT_BOOL:
> -                if ( !parse_bool(optval) )
> +                rc = parse_bool(optval);
> +                if ( rc == -1 )

Maybe "rc < 0"?

> @@ -131,13 +147,21 @@ static void __init _cmdline_parse(const char *cmdline)
>                      safe_strcpy(opt, "no");
>                      optval = opt;
>                  }
> -                ((void (*)(const char *))param->var)(optval);
> +                rc = ((int (*)(const char *))param->var)(optval);
>                  break;
>              default:
>                  BUG();
>                  break;
>              }
> +
> +            break;
>          }
> +
> +        if ( rc )
> +            printk("parameter \"%s\" has invalid value \"%s\"!\n", optkey,
> +                   optval);

With the changes made to optval in OPT_CUSTOM handling this
may end up being confusing.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel