On Fri, Oct 27, 2023 at 1:12 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> wrote:
On 2023/10/27 12:44, lijiang wrote:
> On Wed, Oct 25, 2023 at 4:09 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
> wrote:
>
>> On 2023/10/24 18:10, Lianbo Jiang wrote:
>>> This is a backport patch from gdb commit 58abdf887821 ("Verify COFF
>>> symbol stringtab offset").
>>>
>>> The AddressSanitizer reports a heap-use-after-free error as below:
>>> gdb/coff-pe-read.c:137:27
>>>
>>> Add a COFF offset check to fix this issue.
>>>
>>> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=30640
>>> Signed-off-by: Lianbo Jiang <lijiang@xxxxxxxxxx>
>>
>> Thank you for working on this issue.
>>
>>> ---
>>> Please see the CVE-2023-39129.
>>>
>>> gdb-10.2.patch | 31 +++++++++++++++++++++++++++++++
>>> 1 file changed, 31 insertions(+)
>>>
>>> diff --git a/gdb-10.2.patch b/gdb-10.2.patch
>>> index 7f4b86350bde..98538dc1138b 100644
>>> --- a/gdb-10.2.patch
>>> +++ b/gdb-10.2.patch
>>> @@ -3156,4 +3156,35 @@ exit 0
>>> + else if (i >= 0 && encoded[i] == '$')
>>> len0 = i;
>>> }
>>> +
>>
>> No actual problem, but this empty '+' line is some strange... my working
>
>
> Thank you for the comment, Kazu.
>
> After removing the empty "+" line, I got the following information during
> compiling process.
> ...
> patching file gdb-10.2/gdb/coffread.c
> Hunk #3 succeeded at 1318 with fuzz 1.
yes, because there is need to shift a line like this:
Good to me, thanks.
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -3157,3 +3157,33 @@ exit 0
len0 = i;
}
+--- gdb-10.2/gdb/coffread.c.orig
++++ gdb-10.2/gdb/coffread.c
> ...
>
> But, maybe we can ignore the above output.
>
>
>>
>> tree at a8e5e4cbae54 already has a space line at the end of file:
>>
>> $ tail -n 5 gdb-10.2.patch | tr ' ' '-'
>> -------else-if-(encoded[i]-==-'$')
>> +------else-if-(i->=-0-&&-encoded[i]-==-'$')
>> ---------len0-=-i;
>> -----}
>> -
>>
>> but anyway, I can fix this while merging.
>>
>> Ok.
>
>
>>> +--- gdb-10.2/gdb/coffread.c.orig
>>> ++++ gdb-10.2/gdb/coffread.c
>>> +@@ -159,6 +159,7 @@ static long linetab_offset;
>>> + static unsigned long linetab_size;
>>> +
>>> + static char *stringtab = NULL;
>>> ++static long stringtab_length = 0;
>>> +
>>> + extern void stabsread_clear_cache (void);
>>> +
>>> +@@ -1297,6 +1298,7 @@ init_stringtab (bfd *abfd, long offset,
>> gdb::unique_xmalloc_ptr<char> *storage)
>>> + /* This is in target format (probably not very useful, and not
>>> + currently used), not host format. */
>>> + memcpy (stringtab, lengthbuf, sizeof lengthbuf);
>>> ++ stringtab_length = length;
>>> + if (length == sizeof length) /* Empty table -- just the count.
>> */
>>> + return 0;
>>> +
>>> +@@ -1316,8 +1318,9 @@ getsymname (struct internal_syment *symbol_entry)
>>> +
>>> + if (symbol_entry->_n._n_n._n_zeroes == 0)
>>> + {
>>> +- /* FIXME: Probably should be detecting corrupt symbol files by
>>> +- seeing whether offset points to within the stringtab. */
>>> ++ if (symbol_entry->_n._n_n._n_offset > stringtab_length)
>>> ++ error (_("COFF Error: string table offset (%ld) outside string
>> table (length %ld)"),
>>> ++ symbol_entry->_n._n_n._n_offset, stringtab_length);
>>> + result = stringtab + symbol_entry->_n._n_n._n_offset;
>>> + }
>>> + else
>>>
>>
>> Just out of curiosity, do you have any information that crash can be
>
>
> I did not reproduce it in crash-utility, and got the bug from the reporter.
OK, thanks.
>
>
>>
>> affected by this issue? I don't know about the COFF, I wonder if the
>
>
> Me too.
>
> embedded gdb does not use this, aside from an independent gdb..
>>
>>
> I just saw that the coff format was removed from gcc 8, so probably the gdb
> does not use the coff format.
>
> I copied the following text from the link:
> ---
> Caveats
> Support for the obsolete SDB/coff debug info format has been removed. The
> option -gcoff no longer does anything.
> ...
> ---
> Here is the link: https://gcc.gnu.org/gcc-8/changes.html
>
> But I'm not sure if there are people who might compile crash-utility with
> the old gcc( eg. < gcc 8), if no, we can drop this patch, and no need to
> fix it. Any thoughts?
I think that what compiles crash doesn't matter. Apparently the issue
was reproduced by a crafted debuginfo.
https://inbox.sourceware.org/gdb-patches/20230822152335.231921-1-keiths@xxxxxxxxxx/T/#u
Crash may not be affected with crash-specific checks, but it will be
very hard to prove it. GDB maintainers decided to fix this, let's have
it in the same way.. Hopefully it's removed in future :)
So applied.
https://github.com/crash-utility/crash/commit/fc6ed525407ffc6a181ab9b902f2fb23936309d2
Ok, thanks.
Lianbo
Thanks,
Kazu
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
