Re: [PATCH] xen/cmdline: Fix buggy strncmp(s, LITERAL, ss - s) construct

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 04/01/2019 15:32, Jan Beulich wrote:
>>>> On 31.12.18 at 18:35, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/cpu/vpmu.c
>> +++ b/xen/arch/x86/cpu/vpmu.c
>> @@ -61,42 +61,31 @@ static unsigned vpmu_count;
>>  
>>  static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
>>  
>> -static int parse_vpmu_param(const char *s, unsigned int len)
>> -{
>> -    if ( !*s || !len )
>> -        return 0;
>> -    if ( !strncmp(s, "bts", len) )
>> -        vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
>> -    else if ( !strncmp(s, "ipc", len) )
>> -        vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
>> -    else if ( !strncmp(s, "arch", len) )
>> -        vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
>> -    else
>> -        return 1;
>> -    return 0;
>> -}
>> -
>>  static int __init parse_vpmu_params(const char *s)
>>  {
>> -    const char *sep, *p = s;
>> +    const char *ss;
>>  
>>      switch ( parse_bool(s, NULL) )
>>      {
>>      case 0:
>>          break;
>>      default:
>> -        for ( ; ; )
>> -        {
>> -            sep = strchr(p, ',');
>> -            if ( sep == NULL )
>> -                sep = strchr(p, 0);
>> -            if ( parse_vpmu_param(p, sep - p) )
>> -                goto error;
>> -            if ( !*sep )
>> -                /* reached end of flags */
>> -                break;
>> -            p = sep + 1;
>> -        }
>> +        do {
>> +            ss = strchr(s, ',');
>> +            if ( !ss )
>> +                ss = strchr(s, '\0');
>> +
>> +            if ( !cmdline_strcmp(s, "bts") )
>> +                vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
>> +            else if ( !cmdline_strcmp(s, "ipc") )
>> +                vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
>> +            else if ( !cmdline_strcmp(s, "arch") )
>> +                vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
>> +            else
>> +                return -EINVAL;
>> +
>> +            s = ss + 1;
>> +        } while ( *ss );
> While presumably also applicable elsewhere, the issue is more
> noticeable here because you introduce "ss" anew:

I don't introduce it.  It was previously named "sep", which I renamed
for consistency.  It is also subtly disguised with a 0 instead of '\0'
and a redundant local variable named p.

> It is now
> unhelpful (in terms of generated code) to calculate ss before
> the various cmdline_strcmp() calls, as the compiler can't know
> (despite the const) that what s points to won't change across
> those calls, and hence has to calculate ss early (and put it into
> a callee saved register or on the stack), as written. If the
> calculation was pulled down, only scratch registers would
> suffice for the compiler to carry out the calculation.
>
> That said - all of this is boot time only code, so not really
> performance critical. It's just that this general structure will
> then further proliferate, and the overall binary size is likely
> going to be (slightly) larger this way.

TBH, I'm not concerned far more with code consistency than efficiency,
given how often we manage to subtly break the command line parsing.

If we do want to come up with a more efficient way of doing this, we can
do a blanket change to our prevailing style.

>
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -227,19 +227,49 @@ int parse_bool(const char *s, const char *e)
>>      if ( !len )
>>          return -1;
>>  
>> -    if ( !strncmp("no", s, len) ||
>> -         !strncmp("off", s, len) ||
>> -         !strncmp("false", s, len) ||
>> -         !strncmp("disable", s, len) ||
>> -         !strncmp("0", s, len) )
>> -        return 0;
>> +    switch ( len )
>> +    {
>> +    case 1:
>> +        if ( *s == '1' )
>> +            return 1;
>> +        else if ( *s == '0' )
> The "else" here is pointless (also further down).

So they are.  I'll drop.

>
>> @@ -271,6 +301,29 @@ int parse_boolean(const char *name, const char *s, const char *e)
>>      return -1;
>>  }
>>  
>> +int cmdline_strcmp(const char *frag, const char *name)
>> +{
>> +    while ( 1 )
> Could I talk you into using "for ( ; ; )" instead (and then perhaps
> moving the two increments up here)? I know gcc doesn't do this,
> but in the general case a compiler warning about such constant
> conditionals is not an entirely bad or wrong thing, so I prefer to
> see such constructs avoided where we reasonably can.

Ok.

>
>> +    {
>> +        int res = (*frag - *name);
> With the result of this being implementation defined (due to plain
> char's implementation defined - often command line controlled
> with an implementation defined default - signedness) I wonder if
> this function can really usefully return "int" rather than "bool".

My CPUID command line parsing needs this to work properly as int, for
bisecting across a sorted list.

I'll add an explicit cast to signed char.  I'll also fix out local libc
functions, which are similarly buggy.

>
>> +        if ( res || *name == '\0' )
>> +        {
>> +            /*
>> +             * NUL in 'name' matching a comma or colon in 'frag' implies
>> +             * success.
>> +             */
>> +            if ( *name == '\0' && (*frag == ',' || *frag == ':') )
> There's only a single (unrelated) use of ; as a separator right
> now (afaics), but adding it here would seem quite desirable to
> me.

Where is ; used, out of interest?  I'm happy to add it, but I didn't
spot it on my audit.

> Also, speaking of (the lack of) tokenization of the command line
> in the caller, wouldn't it make sense to accept white space as
> separators here too?

I'm not sure if that is wise or not.  I can't think of a situation where
you would want that behaviour, rather than finding yourself with a
parsing bug and having to fix a bug elsewhere.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel