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.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Wei Liu <wei.l...@citrix.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Stefano Stabellini <sstabell...@kernel.org>
>> CC: Julien Grall <julien.gr...@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
