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

[Juergen Gross <jgross@xxxxxxxx>]

On 14/08/17 14:46, Jan Beulich wrote:
>>>> 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.

Hmm, okay.

> 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.

Okay, I'll have a try.

> Also - stray blanks inside the outermost parentheses.
> 
> And finally, wouldn't it be better to check for overflow _before_
> assigning to *param->var?

I didn't want to change existing behavior. OTOH thinking twice you are
right. Better using the default value than an unexpected small one.

> 
>> @@ -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).

I thought about using a union in struct kernel_param and removing
above type cast. This would require modifying the initialization of
the kernel_param struct via the *_param() macros, though.

The other possibility would be using __builtin_types_compatible_p()
to check the function to be of proper type.

What would you like best?

> 
>>                      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.

I wasn't aware of such a usage.

I'm fine for both alternatives. As you seem to prefer to keep support
for multiple handlers I'll modify the patch to allow that.

>> @@ -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"?

Okay.

> 
>> @@ -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.

Oh yes, good catch.


Juergen


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