This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [RISCV] Use after free in gas/config/tc-riscv.c: riscv_set_arch()
- From: Andrew Waterman <andrew at sifive dot com>
- To: "Klaus Kruse Pedersen (Klaus)" <klauskpedersen at rdamicro dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Wed, 26 Jul 2017 00:24:26 -0700
- Subject: Re: [RISCV] Use after free in gas/config/tc-riscv.c: riscv_set_arch()
- Authentication-results: sourceware.org; auth=none
- References: <1501044502.7598.3.camel@rdamicro.com> <CA++6G0BDWR9VynD=OJMxyhc9KEU=4Rzn68ocMJzZGay_iQSOxA@mail.gmail.com> <1501048785.7598.5.camel@rdamicro.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
>>
>>