Re: [RISCV] Use after free in gas/config/tc-riscv.c: riscv_set_arch()

[Andrew Waterman <andrew at sifive dot com>]

Indeed, I can't see why any standard extension would be special-cased.
I'll contact the author of the Q patch to make sure we aren't missing
anything.

On Tue, Jul 25, 2017 at 10:59 PM, Klaus Kruse Pedersen (Klaus)
<klauskpedersen@rdamicro.com> wrote:
> On Tue, 2017-07-25 at 22:08 -0700, Andrew Waterman wrote:
>> Hi Klaus,
>>
>> Thanks for pointing this out.  We'll submit a patch shortly, tail
>> between legs.
>
> Andrew,
>
> Actually, just below there is something that look like another bug. The
> separate check for 'q' seem to suggest that 'q' can be specified at any
> position in the arch string:
>
>  150 const char *all_subsets = "imafdc";
> [...]
>  207       else if ((all_subsets = strchr (all_subsets, *p)) != NULL)
>  208         {
>  209           const char subset[] = {*p, 0};
>  210           riscv_add_subset (subset);
>  211           all_subsets++;
>  212           p++;
>  213         }
>  214       else if (*p == 'q')
>  215         {
>  216           const char subset[] = {*p, 0};
>  217           riscv_add_subset (subset);
>  218           p++;
>  219         }
>
> Now, lets say you pass: '...qc'. When matching 'q' strchr() will not
> find 'q' in all_subsets and return NULL; and the 'if (*p == 'q')'
> branch is executed as expected.
> But all_subsets is now NULL, so the 'c' can't be matched in the next
> iteration.
>
> So implicitly 'q' have to be the last subset. i.e. 'imfadcq'.
>
> But then there is no reason for the special case:
>
>  150 const char *all_subsets = "imafdcq";
> [...]
>  207       else if
> ((all_subsets = strchr (all_subsets, *p)) != NULL)
>  208         {
>  209
>         const char subset[] = {*p, 0};
>  210           riscv_add_subset
> (subset);
>  211           all_subsets++;
>  212           p++;
>  213         }
>
>
> Right?
>
>
> Klaus
>
>
>
>
>
>>
>> Andrew
>>
>> On Tue, Jul 25, 2017 at 9:48 PM, Klaus Kruse Pedersen (Klaus)
>> <klauskpedersen@rdamicro.com> wrote:
>> >
>> >
>> > The code below will pass a pointer to a free'd string to as_fatal()
>> > when more than one extension is been specified:
>> >
>> >  151 const char *extension = NULL;
>> > [...]
>> >  187   while (*p)
>> >  188     {
>> >  189       if (*p == 'x')
>> >  190         {
>> >  191           char *subset = xstrdup (p), *q = subset;
>> >  192
>> >  193           while (*++q != '\0' && *q != '_')
>> >  194             ;
>> >  195           *q = '\0';
>> >  196
>> >  197           if (extension)
>> >  198             as_fatal ("-march=%s: only one non-standard
>> > extension
>> > is supported"
>> >  199                       " (found `%s' and `%s')", s, extension,
>> > subset);
>> >
>> > Problem is here:
>> >
>> >  200           extension = subset;
>> >  201           riscv_add_subset (subset);
>> >  202           p += strlen (subset);
>> >  203           free (subset);
>> >  204         }
>> > [...]
>> >
>> > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=g
>> > as/c
>> > onfig/tc-
>> > riscv.c;h=55c41c5db3c9240e8da3cdf8906cfb745d041c6f;hb=HEAD#l187
>>
>>