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

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

On 02/01/2019 12:25, Roger Pau Monné wrote:
> On Wed, Jan 02, 2019 at 12:18:27PM +0000, Andrew Cooper wrote:
>> 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.
> But shouldn't you check for *frag == NUL in order to avoid overruns?

That check is present, but I'll admit that it is a subtlety of how
strcmp() works.  A NUL in frag matching anything other than a NUL in
name will cause res to be nonzero and exit via that path.  This
particular arrangement of the function deliberately doesn't increment
the frag pointer until later in the loop.

~Andrew

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