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

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

On 02/01/2019 10:13, Roger Pau Monné wrote:
> On Mon, Dec 31, 2018 at 05:35:21PM +0000, Andrew Cooper wrote:
>> When the command line parsing was updated to use const strings and no longer
>> tokenise with NUL characters, string matches could no longer be made with
>> strcmp().
>>
>> Unfortunately, the replacement was buggy.  strncmp(s, "opt", ss - s) matches
>> "o", "op" and "opt" on the command line, as ss - s may be shorter than the
>> passed literal.  Furthermore, parse_bool() is affected by this, so substrings
>> such as "d", "e" and "o" are considered valid, with the latter being ambiguous
>> between "on" and "off".
>>
>> Introduce a new strcmp-like function for the task, which looks for exact
>> string matches, but declares success when the NUL of the literal matches a
>> comma or colon in the command line fragment.
>>
>> No change to the intended parsing functionality, but fixes cases where a
>> partial string on the command line will inadvertently trigger options.
>>
>> A few areas were more than just a trivial change:
>>
>>  * fdt_add_uefi_nodes(), while not command line parsing, had the same broken
>>    strncmp() pattern.  As a fix, perform an explicit length check first.
>>  * parse_irq_vector_map_param() gained some style corrections.
>>  * parse_vpmu_params() was rewritten to use the normal list-of-options form,
>>    rather than just fixing up parse_vpmu_param() and leaving the parsing being
>>    hard to follow.
>>  * Instead of making the trivial fix of adding an explicit length check in
>>    parse_bool(), use the length to select which token to we search for, which
>>    is more efficient than the previous linear search over all possible tokens.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> Split out of the dom0 fix series.  This needs backporting to 4.9 and later,
>> and to the security trees, as this bug has been backported in security fixes.
>>
>> This patch is more easily reviewed with `git diff --color-words` which
>> highlights that it is a straight function transformation in most cases.
>>
>> The psr= option is a complete pain, and unlike all similar options in Xen.
>> I've half a mind to rewrite it from scratch, seeing as the option isn't
>> enabled by default.
>> ---
>> +int cmdline_strcmp(const char *frag, const char *name)
>> +{
>> +    while ( 1 )
>> +    {
>> +        int res = (*frag - *name);
>> +
>> +        if ( res || *name == '\0' )
>> +        {
>> +            /*
>> +             * NUL in 'name' matching a comma or colon in 'frag' implies
>> +             * success.
>> +             */
>> +            if ( *name == '\0' && (*frag == ',' || *frag == ':') )
>> +                res = 0;
>> +
>> +            return res;
>> +        }
>> +
>> +        frag++;
>> +        name++;
>> +    }
>> +}
> The previous function would get the max length of the frag argument,
> which I think was useful. If the length of name > frag you could end
> up accessing an unmapped address AFAICT. Or at least *frag == '\0'
> should also be taken into account if it's guaranteed that frag must
> always have an ending NUL.

It is completely unreasonable to pass a non-terminated string into
string parsing functions, and a lot of the parsing code will explode if
this expectation is violated.

Remember that before the const parsing was introduced (4.9 iirc), all
parsing went without a max length, and resolving that is part of the bugfix.

> I would also consider adding __attribute__ ((format_arg (2))); to the
> prototype, so that the name argument is always checked to be a
> literal, as is the current usage.

That would falsely prohibit looking for a string with a literal % in it.

Furthermore, see "[PATCH 3/9] x86/cpuid: Extend the cpuid= command line
option to support all named features" which was the origin of this
function, and a usecase where it definitely won't have a string literal.

~Andrew

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