From: "Willgerodt, Felix" <felix.willgerodt@intel.com>
To: Andrew Burgess <aburgess@redhat.com>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH 1/1] gdb: Fix segfault in "start" with fuzzed binary
Date: Mon, 8 Apr 2024 09:20:04 +0000 [thread overview]
Message-ID: <MN2PR11MB456672BB41D76ADBA113C57C8E002@MN2PR11MB4566.namprd11.prod.outlook.com> (raw)
In-Reply-To: <87a5m52s32.fsf@redhat.com>
> -----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
prev parent reply other threads:[~2024-04-08 9:20 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-19 15:19 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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MN2PR11MB456672BB41D76ADBA113C57C8E002@MN2PR11MB4566.namprd11.prod.outlook.com \
--to=felix.willgerodt@intel.com \
--cc=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).