* [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary @ 2024-02-19 15:19 Felix Willgerodt 2024-02-20 11:57 ` Andrew Burgess 0 siblings, 1 reply; 5+ messages in thread From: Felix Willgerodt @ 2024-02-19 15:19 UTC (permalink / raw) To: gdb-patches 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. But after this patch, the segfault is gone and the output seems reasonable for a broken binary: Temporary breakpoint 1 at 0x1040: file main.c, line 1. Starting program: a.out /bin/bash: line 1: a.out: Permission denied /bin/bash: line 1: exec: a.out: cannot execute: Permission denied During startup program exited with code 126. (gdb) In addition this changes the function to use explicit comparisons. --- gdb/breakpoint.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 5f05657a8b3..0313b920ca6 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -7641,9 +7641,9 @@ set_breakpoint_location_function (struct bp_location *loc) struct gdbarch * get_sal_arch (struct symtab_and_line sal) { - if (sal.section) + if (sal.section != nullptr && sal.section->objfile != nullptr) return sal.section->objfile->arch (); - if (sal.symtab) + if (sal.symtab != nullptr) return sal.symtab->compunit ()->objfile ()->arch (); return NULL; -- 2.34.1 Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary 2024-02-19 15:19 [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary Felix Willgerodt @ 2024-02-20 11:57 ` Andrew Burgess 2024-02-20 14:13 ` Willgerodt, Felix 0 siblings, 1 reply; 5+ messages in thread From: Andrew Burgess @ 2024-02-20 11:57 UTC (permalink / raw) To: Felix Willgerodt, gdb-patches 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. > But after this patch, the segfault > is gone and the output seems reasonable for a broken binary: > > Temporary breakpoint 1 at 0x1040: file main.c, line 1. > Starting program: a.out > /bin/bash: line 1: a.out: Permission denied > /bin/bash: line 1: exec: a.out: cannot execute: Permission denied > During startup program exited with code 126. Not ideal, but I better than a crash :) Thanks, Andrew > (gdb) > > In addition this changes the function to use explicit comparisons. > --- > gdb/breakpoint.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 5f05657a8b3..0313b920ca6 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -7641,9 +7641,9 @@ set_breakpoint_location_function (struct bp_location *loc) > struct gdbarch * > get_sal_arch (struct symtab_and_line sal) > { > - if (sal.section) > + if (sal.section != nullptr && sal.section->objfile != nullptr) > return sal.section->objfile->arch (); > - if (sal.symtab) > + if (sal.symtab != nullptr) > return sal.symtab->compunit ()->objfile ()->arch (); > > return NULL; > -- > 2.34.1 > > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary 2024-02-20 11:57 ` Andrew Burgess @ 2024-02-20 14:13 ` Willgerodt, Felix 2024-04-07 13:57 ` Andrew Burgess 0 siblings, 1 reply; 5+ messages in thread From: Willgerodt, Felix @ 2024-02-20 14:13 UTC (permalink / raw) To: Andrew Burgess, gdb-patches > -----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?) Regards, Felix Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary 2024-02-20 14:13 ` Willgerodt, Felix @ 2024-04-07 13:57 ` Andrew Burgess 2024-04-08 9:20 ` Willgerodt, Felix 0 siblings, 1 reply; 5+ messages in thread From: Andrew Burgess @ 2024-04-07 13:57 UTC (permalink / raw) To: Willgerodt, Felix, gdb-patches "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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary 2024-04-07 13:57 ` Andrew Burgess @ 2024-04-08 9:20 ` Willgerodt, Felix 0 siblings, 0 replies; 5+ messages in thread From: Willgerodt, Felix @ 2024-04-08 9:20 UTC (permalink / raw) To: Andrew Burgess, gdb-patches > -----Original Message----- > From: Andrew Burgess <aburgess@redhat.com> > Sent: Sonntag, 7. April 2024 15: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 > > "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 Thanks for explaining this. I double checked it again and you are correct. Not sure what I was debugging last time, sorry for the confusion. When I break in add_to_objfile_sections() and the section is ".text", I do indeed see the early return being taken. That makes me wonder. In bfd/section.c I read this: . {* Tells the OS to allocate space for this section when loading. . This is clear for a section containing debug information only. *} .#define SEC_ALLOC 0x1 Which sounds like there could be debug info in non-allocatable sections? Doesn't that mean we should just fill out the section details in objfile.c regardless? Though this might be too naive, as I am not too familiar with the objfile code. When I remove the allocatable check in add_to_objfile_sections(), the testsuite still runs reasonably fine for me on x86 Ubuntu 22.04. Though there is a regression in gdb.base/maint-info-sections.exp. As there are now new sections and as the test checks for the ALLOC flag. And my fuzzed binary doesn't crash anymore either: $ gdb -q -ex "start" a.out Reading symbols from a.out... Temporary breakpoint 1 at 0x1040: file main.c, line 1. Starting program: a.out /bin/bash: line 1: a.out: Permission denied /bin/bash: line 1: exec: a.out: cannot execute: Permission denied During startup program exited with code 126. (gdb) quit $ ./a.out -bash: ./a.out: Permission denied $ echo $? 126 Regards, Felix Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-08 9:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-19 15:19 [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary Felix Willgerodt 2024-02-20 11:57 ` Andrew Burgess 2024-02-20 14:13 ` Willgerodt, Felix 2024-04-07 13:57 ` Andrew Burgess 2024-04-08 9:20 ` Willgerodt, Felix
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).