RE: [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary

[Andrew Burgess <aburgess@redhat.com>]

"Willgerodt, Felix" <felix.willgerodt@intel.com> writes:

>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Dienstag, 20. Februar 2024 12:58
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
>> Subject: Re: [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary
>> 
>> Felix Willgerodt <felix.willgerodt@intel.com> writes:
>> 
>> > I found this while playing around with fuzzing.  Though I did fuzz with
>> > "-ex start", so this isn't a security issue.  But any comments welcome.
>> >
>> > What I observed is this segfault:
>> >
>> > ~~~
>> > Thread 1 "gdb-up" received signal SIGSEGV, Segmentation fault.
>> > 0x0000555555d8883a in objfile::arch (this=0x0) at
>> /user/sources/gdb/gdb/objfiles.h:509
>> > 509         return per_bfd->gdbarch;
>> > (gdb) bt 5
>> >     filter_=..., cond_string_=..., extra_string_=..., disposition_=disp_del,
>> thread_=-1, simd_lane_num_=-1, task_=-1, inferior_=1, ignore_count_=0,
>> >     from_tty=0, enabled_=1, flags=0, display_canonical_=0) at
>> /user/sources/gdb/gdb/breakpoint.c:8960
>> > (More stack frames follow...)
>> > (gdb) frame 1
>> > 7645        return sal.section->objfile->arch ();
>> > (gdb) p sal.section
>> > $1 = (obj_section *) 0x555558828bf8
>> > (gdb) p *sal.section
>> > $2 = {the_bfd_section = 0x0, objfile = 0x0, ovly_mapped = 0}
>> > ~~~
>> >
>> > The parsed binary has a weird .text section header:
>> >
>> >   [14] .text             LOUSER+0x6c0000  0000000000001040  00001040
>> >        00000000000000f9  0000000000000000  WX       0     0     16
>> >
>> > It is marked as writeable (I think) and the type is also different.  For
>> > reference here is the one from the normal binary that I started fuzzing with:
>> >
>> >   [14] .text             PROGBITS         0000000000001040  00001040
>> >        00000000000000f9  0000000000000000  AX       0     0     16
>> >
>> > I couldn't find where GDB actually parses this.  Nor could I figure out why
>> > the section has a nullptr as objfile.
>> 
>> So I think add_to_objfile_sections is where the nullptr is appearing.
>> This is where the obj_section::objfile field is set, but only if the
>> section is allocatable, which after your fuzzing it's not, so the
>> ::objfile field ends up being left as its default value.
>> 
>> Even after this patch, it's not obvious that the ::objfile field might
>> be nullptr (looking at struct obj_section in objfiles.h), so maybe it's
>> worth extending the comment there to reflect that.
>> 
>> I did wonder if we're wrong to even create a symtab_and_line in this
>> case, we're claiming to have found some debug information for the
>> program image from a particular section which actually wasn't mapped
>> in.  But I think fixing that would be a much bigger task, we'd need to
>> chase back all the places where we load debug information which claims
>> to be within a section which is then not going to be allocated.
>> 
>
> Hi Andrew,
>
> thanks for even looking at this. Since it isn't a security issue I was wondering
> if we even care much about this. I don't really know if we would ever see
> such an ELF file and care about not crashing with it after a start.
> But the patch I wrote did seem harmless enough to post as a proposal.
>
> I did check add_to_objfile_sections() and I don't see the nullptr being added there.
> So it must be somewhere else. (Wouldn't it even segfault there if objfile
> would be nullptr?)

When you say you don't see the nullptr being added, what do you mean
exactly?

As far as I can tell, this is where obj_section::objfile is set from
nullptr to non-nullptr, but that only happens for allocatable sections.

In your case you specifically said the fuzzer made the section
non-allocatable, so that assignment of obj_section::objfile will not
happen, and obj_section::objfile will be left with its default (nullptr)
value.

At least, that's my thinking.  I haven't actually tested this, so
possibly I'm not understanding something!

Thanks,
Andrew