[review] DWARF reader: Reject sections with invalid sizes

[review] DWARF reader: Reject sections with invalid sizes

[Keith Seitz (Code Review)]

Keith Seitz has uploaded a new change for review.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127
......................................................................

DWARF reader: Reject sections with invalid sizes

This is another fuzzer bug, gdb/23567.  This time, the fuzzer has
specifically altered the size of .debug_str:

$ eu-readelf -S objdump
Section Headers:
[Nr] Name                 Type         Addr             Off      Size     ES Flags Lk Inf Al
[31] .debug_str           PROGBITS     0000000000000000 0057116d ffffffffffffffff  1 MS     0   0  1

When this file is loaded into GDB, the DWARF reader crashes attempting
to access the string table (or it may just store a bunch of nonsense):

[gdb-8.3-6-fc30]
$ gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
Segmentation fault (core dumped)

Nick has already committed a BFD patch to issue the warning seen above.

[gdb master 6acc1a0b]
$ gdb -BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
(gdb) inf func
All defined functions:

File ./../include/dwarf2.def:
186:	const

              8 *>(.:
                     ;'@�B);
747:	const

              8 *�(.:
                     ;'@�B);
701:	const

              8 *�D �
                     (.:
                        ;'@�B);
71:	const

              8 *(.:
                    ;'@�B);
/* and more gibberish  */

Consider read_indirect_string_at_offset_from:

static const char *
read_indirect_string_at_offset_from (struct objfile *objfile,
                                     bfd *abfd, LONGEST str_offset,
                                     struct dwarf2_section_info *sect,
                                     const char *form_name,
                                     const char *sect_name)
{
  dwarf2_read_section (objfile, sect);
  if (sect->buffer == NULL)
    error (_("%s used without %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  if (str_offset >= sect->size)
    error (_("%s pointing outside of %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  gdb_assert (HOST_CHAR_BIT == 8);
  if (sect->buffer[str_offset] == '\0')
    return NULL;
  return (const char *) (sect->buffer + str_offset);
}

With sect_size being ginormous, the code attempts to access
sect->buffer[GINORMOUS], and depending on the layout of memory,
GDB either stores a bunch of gibberish strings or crashes.

This is an attempt to mitigate this by implementing a similar approach
used by BFD. In our case, we simply reject the section with the invalid
length:

$ ./gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...

warning: Discarding section .debug_str which has a section size (ffffffffffffffff) larger than the file size [in module /path/to/objdump]
DW_FORM_strp used without .debug_str section [in module /path/to/objdump]
(No debugging symbols found in /path/to/objdump)
(gdb)

Unfortunately, I have not found a way to regression test this, since it
requires poking ELF section headers.

gdb/ChangeLog:
2019-10-16  Keith Seitz  <keiths@redhat.com>

	PR gdb/23567
	* dwarf2read.c (dwarf2_per_objfile::locate_sections): Discard
	sections whose size is greater than the file size.

Change-Id: I896ac3b4eb2207c54e8e05c16beab3051d9b4b2f
---
M gdb/dwarf2read.c
1 file changed, 9 insertions(+), 0 deletions(-)



diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0443b55..997726c 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2338,6 +2338,15 @@
   if ((aflag & SEC_HAS_CONTENTS) == 0)
     {
     }
+  else if (elf_section_data (sectp)->this_hdr.sh_size
+	   > bfd_get_file_size (abfd))
+    {
+      warning (_("Discarding section %s which has a section size (%"
+	       BFD_VMA_FMT "x) larger than the file size [in module %s]"),
+	     bfd_section_name (sectp),
+	     elf_section_data (sectp)->this_hdr.sh_size,
+	     bfd_get_filename (abfd));
+    }
   else if (section_is_p (sectp->name, &names.info))
     {
       this->info.s.section = sectp;

[review] DWARF reader: Reject sections with invalid sizes

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127
......................................................................


Patch Set 1:

(1 comment)

Looks good but see the note.  Thanks for doing this.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127/1/gdb/dwarf2read.c 
File gdb/dwarf2read.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127/1/gdb/dwarf2read.c@2345 
PS1, Line 2345: 	       BFD_VMA_FMT "x) larger than the file size [in module %s]"),
BFD can set this to "I64", which isn't supported by gdbsupport/format.c.  Maybe that should be fixed, but just using one of the gdb helpers like phex or whatever seems simpler... I don't see other uses of BFD_VMA_FMT in gdb.

[review] DWARF reader: Reject sections with invalid sizes

[Keith Seitz (Code Review)]

Keith Seitz has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127
......................................................................


Patch Set 2:

(1 comment)

Switched to phex_nz. Thanks for the review.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127/1/gdb/dwarf2read.c 
File gdb/dwarf2read.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127/1/gdb/dwarf2read.c@2345 
PS1, Line 2345: 	       BFD_VMA_FMT "x) larger than the file size [in module %s]"),
> BFD can set this to "I64", which isn't supported by gdbsupport/format.c. […]
Done

[review] DWARF reader: Reject sections with invalid sizes

[Keith Seitz (Code Review)]

Keith Seitz has uploaded a new patch set version (#2).

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127
......................................................................

DWARF reader: Reject sections with invalid sizes

This is another fuzzer bug, gdb/23567.  This time, the fuzzer has
specifically altered the size of .debug_str:

$ eu-readelf -S objdump
Section Headers:
[Nr] Name                 Type         Addr             Off      Size     ES Flags Lk Inf Al
[31] .debug_str           PROGBITS     0000000000000000 0057116d ffffffffffffffff  1 MS     0   0  1

When this file is loaded into GDB, the DWARF reader crashes attempting
to access the string table (or it may just store a bunch of nonsense):

[gdb-8.3-6-fc30]
$ gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
Segmentation fault (core dumped)

Nick has already committed a BFD patch to issue the warning seen above.

[gdb master 6acc1a0b]
$ gdb -BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
(gdb) inf func
All defined functions:

File ./../include/dwarf2.def:
186:	const

              8 *>(.:
                     ;'@�B);
747:	const

              8 *�(.:
                     ;'@�B);
701:	const

              8 *�D �
                     (.:
                        ;'@�B);
71:	const

              8 *(.:
                    ;'@�B);
/* and more gibberish  */

Consider read_indirect_string_at_offset_from:

static const char *
read_indirect_string_at_offset_from (struct objfile *objfile,
                                     bfd *abfd, LONGEST str_offset,
                                     struct dwarf2_section_info *sect,
                                     const char *form_name,
                                     const char *sect_name)
{
  dwarf2_read_section (objfile, sect);
  if (sect->buffer == NULL)
    error (_("%s used without %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  if (str_offset >= sect->size)
    error (_("%s pointing outside of %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  gdb_assert (HOST_CHAR_BIT == 8);
  if (sect->buffer[str_offset] == '\0')
    return NULL;
  return (const char *) (sect->buffer + str_offset);
}

With sect_size being ginormous, the code attempts to access
sect->buffer[GINORMOUS], and depending on the layout of memory,
GDB either stores a bunch of gibberish strings or crashes.

This is an attempt to mitigate this by implementing a similar approach
used by BFD. In our case, we simply reject the section with the invalid
length:

$ ./gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...

warning: Discarding section .debug_str which has a section size (ffffffffffffffff) larger than the file size [in module /path/to/objdump]
DW_FORM_strp used without .debug_str section [in module /path/to/objdump]
(No debugging symbols found in /path/to/objdump)
(gdb)

Unfortunately, I have not found a way to regression test this, since it
requires poking ELF section headers.

gdb/ChangeLog:
2019-10-16  Keith Seitz  <keiths@redhat.com>

	PR gdb/23567
	* dwarf2read.c (dwarf2_per_objfile::locate_sections): Discard
	sections whose size is greater than the file size.

Change-Id: I896ac3b4eb2207c54e8e05c16beab3051d9b4b2f
---
M gdb/dwarf2read.c
1 file changed, 9 insertions(+), 0 deletions(-)



diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0443b55..a78f818 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2338,6 +2338,15 @@
   if ((aflag & SEC_HAS_CONTENTS) == 0)
     {
     }
+  else if (elf_section_data (sectp)->this_hdr.sh_size
+	   > bfd_get_file_size (abfd))
+    {
+      bfd_size_type size = elf_section_data (sectp)->this_hdr.sh_size;
+      warning (_("Discarding section %s which has a section size (%s"
+		 ") larger than the file size [in module %s]"),
+	       bfd_section_name (sectp), phex_nz (size, sizeof (size)),
+	       bfd_get_filename (abfd));
+    }
   else if (section_is_p (sectp->name, &names.info))
     {
       this->info.s.section = sectp;

[review] DWARF reader: Reject sections with invalid sizes

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127
......................................................................


Patch Set 2: Code-Review+2

Thank you.  This looks good to me!

[review] DWARF reader: Reject sections with invalid sizes

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127
......................................................................

DWARF reader: Reject sections with invalid sizes

This is another fuzzer bug, gdb/23567.  This time, the fuzzer has
specifically altered the size of .debug_str:

$ eu-readelf -S objdump
Section Headers:
[Nr] Name                 Type         Addr             Off      Size     ES Flags Lk Inf Al
[31] .debug_str           PROGBITS     0000000000000000 0057116d ffffffffffffffff  1 MS     0   0  1

When this file is loaded into GDB, the DWARF reader crashes attempting
to access the string table (or it may just store a bunch of nonsense):

[gdb-8.3-6-fc30]
$ gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
Segmentation fault (core dumped)

Nick has already committed a BFD patch to issue the warning seen above.

[gdb master 6acc1a0b]
$ gdb -BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
(gdb) inf func
All defined functions:

File ./../include/dwarf2.def:
186:	const

              8 *>(.:
                     ;'@�B);
747:	const

              8 *�(.:
                     ;'@�B);
701:	const

              8 *�D �
                     (.:
                        ;'@�B);
71:	const

              8 *(.:
                    ;'@�B);
/* and more gibberish  */

Consider read_indirect_string_at_offset_from:

static const char *
read_indirect_string_at_offset_from (struct objfile *objfile,
                                     bfd *abfd, LONGEST str_offset,
                                     struct dwarf2_section_info *sect,
                                     const char *form_name,
                                     const char *sect_name)
{
  dwarf2_read_section (objfile, sect);
  if (sect->buffer == NULL)
    error (_("%s used without %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  if (str_offset >= sect->size)
    error (_("%s pointing outside of %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  gdb_assert (HOST_CHAR_BIT == 8);
  if (sect->buffer[str_offset] == '\0')
    return NULL;
  return (const char *) (sect->buffer + str_offset);
}

With sect_size being ginormous, the code attempts to access
sect->buffer[GINORMOUS], and depending on the layout of memory,
GDB either stores a bunch of gibberish strings or crashes.

This is an attempt to mitigate this by implementing a similar approach
used by BFD. In our case, we simply reject the section with the invalid
length:

$ ./gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...

warning: Discarding section .debug_str which has a section size (ffffffffffffffff) larger than the file size [in module /path/to/objdump]
DW_FORM_strp used without .debug_str section [in module /path/to/objdump]
(No debugging symbols found in /path/to/objdump)
(gdb)

Unfortunately, I have not found a way to regression test this, since it
requires poking ELF section headers.

gdb/ChangeLog:
2019-10-16  Keith Seitz  <keiths@redhat.com>

	PR gdb/23567
	* dwarf2read.c (dwarf2_per_objfile::locate_sections): Discard
	sections whose size is greater than the file size.

Change-Id: I896ac3b4eb2207c54e8e05c16beab3051d9b4b2f
---
M gdb/ChangeLog
M gdb/dwarf2read.c
2 files changed, 15 insertions(+), 0 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6de9f3d..d11dbfb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-10-16  Keith Seitz  <keiths@redhat.com>
+
+	PR gdb/23567
+	* dwarf2read.c (dwarf2_per_objfile::locate_sections): Discard
+	sections whose size is greater than the file size.
+
 2019-10-16  Jim Wilson  <jimw@sifive.com>
 
 	* riscv-tdep.c (riscv_gcc_target_options): New.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0443b55..a78f818 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2338,6 +2338,15 @@
   if ((aflag & SEC_HAS_CONTENTS) == 0)
     {
     }
+  else if (elf_section_data (sectp)->this_hdr.sh_size
+	   > bfd_get_file_size (abfd))
+    {
+      bfd_size_type size = elf_section_data (sectp)->this_hdr.sh_size;
+      warning (_("Discarding section %s which has a section size (%s"
+		 ") larger than the file size [in module %s]"),
+	       bfd_section_name (sectp), phex_nz (size, sizeof (size)),
+	       bfd_get_filename (abfd));
+    }
   else if (section_is_p (sectp->name, &names.info))
     {
       this->info.s.section = sectp;

[review] DWARF reader: Reject sections with invalid sizes

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has uploaded a new patch set version (#3) to the change originally created by Keith Seitz.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/127
......................................................................

DWARF reader: Reject sections with invalid sizes

This is another fuzzer bug, gdb/23567.  This time, the fuzzer has
specifically altered the size of .debug_str:

$ eu-readelf -S objdump
Section Headers:
[Nr] Name                 Type         Addr             Off      Size     ES Flags Lk Inf Al
[31] .debug_str           PROGBITS     0000000000000000 0057116d ffffffffffffffff  1 MS     0   0  1

When this file is loaded into GDB, the DWARF reader crashes attempting
to access the string table (or it may just store a bunch of nonsense):

[gdb-8.3-6-fc30]
$ gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
Segmentation fault (core dumped)

Nick has already committed a BFD patch to issue the warning seen above.

[gdb master 6acc1a0b]
$ gdb -BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
(gdb) inf func
All defined functions:

File ./../include/dwarf2.def:
186:	const

              8 *>(.:
                     ;'@�B);
747:	const

              8 *�(.:
                     ;'@�B);
701:	const

              8 *�D �
                     (.:
                        ;'@�B);
71:	const

              8 *(.:
                    ;'@�B);
/* and more gibberish  */

Consider read_indirect_string_at_offset_from:

static const char *
read_indirect_string_at_offset_from (struct objfile *objfile,
                                     bfd *abfd, LONGEST str_offset,
                                     struct dwarf2_section_info *sect,
                                     const char *form_name,
                                     const char *sect_name)
{
  dwarf2_read_section (objfile, sect);
  if (sect->buffer == NULL)
    error (_("%s used without %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  if (str_offset >= sect->size)
    error (_("%s pointing outside of %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  gdb_assert (HOST_CHAR_BIT == 8);
  if (sect->buffer[str_offset] == '\0')
    return NULL;
  return (const char *) (sect->buffer + str_offset);
}

With sect_size being ginormous, the code attempts to access
sect->buffer[GINORMOUS], and depending on the layout of memory,
GDB either stores a bunch of gibberish strings or crashes.

This is an attempt to mitigate this by implementing a similar approach
used by BFD. In our case, we simply reject the section with the invalid
length:

$ ./gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...

warning: Discarding section .debug_str which has a section size (ffffffffffffffff) larger than the file size [in module /path/to/objdump]
DW_FORM_strp used without .debug_str section [in module /path/to/objdump]
(No debugging symbols found in /path/to/objdump)
(gdb)

Unfortunately, I have not found a way to regression test this, since it
requires poking ELF section headers.

gdb/ChangeLog:
2019-10-16  Keith Seitz  <keiths@redhat.com>

	PR gdb/23567
	* dwarf2read.c (dwarf2_per_objfile::locate_sections): Discard
	sections whose size is greater than the file size.

Change-Id: I896ac3b4eb2207c54e8e05c16beab3051d9b4b2f
---
M gdb/ChangeLog
M gdb/dwarf2read.c
2 files changed, 15 insertions(+), 0 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6de9f3d..d11dbfb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-10-16  Keith Seitz  <keiths@redhat.com>
+
+	PR gdb/23567
+	* dwarf2read.c (dwarf2_per_objfile::locate_sections): Discard
+	sections whose size is greater than the file size.
+
 2019-10-16  Jim Wilson  <jimw@sifive.com>
 
 	* riscv-tdep.c (riscv_gcc_target_options): New.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0443b55..a78f818 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2338,6 +2338,15 @@
   if ((aflag & SEC_HAS_CONTENTS) == 0)
     {
     }
+  else if (elf_section_data (sectp)->this_hdr.sh_size
+	   > bfd_get_file_size (abfd))
+    {
+      bfd_size_type size = elf_section_data (sectp)->this_hdr.sh_size;
+      warning (_("Discarding section %s which has a section size (%s"
+		 ") larger than the file size [in module %s]"),
+	       bfd_section_name (sectp), phex_nz (size, sizeof (size)),
+	       bfd_get_filename (abfd));
+    }
   else if (section_is_p (sectp->name, &names.info))
     {
       this->info.s.section = sectp;