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

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

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.

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.

Thanks, Roger.

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