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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> >> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> CC: Julien Grall <julien.grall@xxxxxxx>
> >>
> >> 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?

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

Oh, didn't know about this case. If the plan is to use it without
literals then the comment is moot.

Thanks, Roger.

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