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

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

[Felix Willgerodt]

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

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

[Andrew Burgess]

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

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

[Willgerodt, Felix]

> -----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

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

[Andrew Burgess]

"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

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

[Willgerodt, Felix]

> -----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