[PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size

[PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size

[Felix Willgerodt]

Consider a binary with an erroneous size of the .dynamic section:

$ readelf a.out
...
  [24] .dynamic          DYNAMIC          0000000000004c20  00003c20
       000000fffffffa40  0000000000000010  WA       7     0     8
...

This binary causes a segfault in GDB, because we pass a negative
value to alloca in gdb_bfd_scan_elf_dyntag.  Alloca accepts size_t though,
so it is really a big size that we pass.  I changed the code to heap
allocation, as the size of the .dynamic section could be "too big" in a
correct binary as well.  That way GDB will assert on a negative size value.

There should be no user visible change after this.
---
 gdb/solib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/solib.c b/gdb/solib.c
index b9fb911a810..45a5309d199 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1546,7 +1546,8 @@ gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
   /* Read in .dynamic from the BFD.  We will get the actual value
      from memory later.  */
   sect_size = bfd_section_size (sect);
-  buf = bufstart = (gdb_byte *) alloca (sect_size);
+  gdb::byte_vector buffer (sect_size);
+  buf = bufstart = buffer.data ();
   if (!bfd_get_section_contents (abfd, sect,
 				 buf, 0, sect_size))
     return 0;
-- 
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 with a negative .dynamic section size

[Keith Seitz]

On 11/10/23 01:16, Felix Willgerodt wrote:
> Consider a binary with an erroneous size of the .dynamic section:
> 
> $ readelf a.out
> ...
>    [24] .dynamic          DYNAMIC          0000000000004c20  00003c20
>         000000fffffffa40  0000000000000010  WA       7     0     8
> ... >
> This binary causes a segfault in GDB, because we pass a negative
> value to alloca in gdb_bfd_scan_elf_dyntag.

Good catch. I especially welcome hardening patches like this,
so thank you!

> Alloca accepts size_t though, so it is really a big size that we
> pass.  I changed the code to heap allocation, as the size of the
> .dynamic section could be "too big" in a correct binary as well.
> That way GDB will assert on a negative size  value.

When you say "will assert", do you mean GDB will actually assert
and abort? If so, that doesn't seem particularly user friendly.

While I welcome the change to using the heap, I have to ask if
you've investigated pushing the fix down to BFD or otherwise attempted
to validate the section size?

Keith

> There should be no user visible change after this.
> ---
>   gdb/solib.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/solib.c b/gdb/solib.c
> index b9fb911a810..45a5309d199 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1546,7 +1546,8 @@ gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
>     /* Read in .dynamic from the BFD.  We will get the actual value
>        from memory later.  */
>     sect_size = bfd_section_size (sect);
> -  buf = bufstart = (gdb_byte *) alloca (sect_size);
> +  gdb::byte_vector buffer (sect_size);
> +  buf = bufstart = buffer.data ();
>     if (!bfd_get_section_contents (abfd, sect,
>   				 buf, 0, sect_size))
>       return 0;

RE: [PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size

[Willgerodt, Felix]

> -----Original Message-----
> From: Keith Seitz <keiths@redhat.com>
> Sent: Freitag, 10. November 2023 20:00
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size
> 
> On 11/10/23 01:16, Felix Willgerodt wrote:
> > Consider a binary with an erroneous size of the .dynamic section:
> >
> > $ readelf a.out
> > ...
> >    [24] .dynamic          DYNAMIC          0000000000004c20  00003c20
> >         000000fffffffa40  0000000000000010  WA       7     0     8
> > ... >
> > This binary causes a segfault in GDB, because we pass a negative
> > value to alloca in gdb_bfd_scan_elf_dyntag.
> 
> Good catch. I especially welcome hardening patches like this,
> so thank you!
> 
> > Alloca accepts size_t though, so it is really a big size that we
> > pass.  I changed the code to heap allocation, as the size of the
> > .dynamic section could be "too big" in a correct binary as well.
> > That way GDB will assert on a negative size  value.
> 
> When you say "will assert", do you mean GDB will actually assert
> and abort? If so, that doesn't seem particularly user friendly.
> 
> While I welcome the change to using the heap, I have to ask if
> you've investigated pushing the fix down to BFD or otherwise attempted
> to validate the section size?
> 
> Keith

Hi Keith,

I double checked this again. It isn't really an assert that we see now.
We get an internal_error in utils.c:malloc_error(). This is the actual output:

GNU gdb (GDB) 15.0.50.20231109-git
Copyright (C) 2023 Free Software Foundation, Inc.                             
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu".
Type "show configuration" for configuration details.   
For bug reporting instructions, please see:                                                    
<https://www.gnu.org/software/gdb/bugs/>.                                                      
Find the GDB manual and other documentation resources online at:                                                                                                                              
    <http://www.gnu.org/software/gdb/documentation/>.
                                               
For help, type "help".                                                                         
Type "apropos word" to search for commands related to "word"...
BFD: warning: bin/poc has a section extending past end of file
Reading symbols from poc...       
                                                                                               
warning: Loadable section ".dynamic" outside of ELF segments
  in bin/poc                                                                    
                                                                                               
warning: Discarding section .dynamic which has a section size (fffffffa40) larger than the file size [in module bin/poc]
(No debugging symbols found in poc)  
sources/gdb/gdb/utils.c:685: internal-error: virtual memory exhausted.
A problem internal to GDB has been detected,
further debugging may prove unreliable.                                                        
----- Backtrace -----   
0x5a114c gdb_internal_backtrace_1                                                              
        sources/gdb/gdb/bt-utils.c:122                                          
0x5a11e4 _Z22gdb_internal_backtracev                                                           
        sources/gdb/gdb/bt-utils.c:168                                          
0xcd0059 internal_vproblem                                                                     
        sources/gdb/gdb/utils.c:396                                             
0xcd03ea _Z15internal_verrorPKciS0_P13__va_list_tag
        sources/gdb/gdb/utils.c:476                                             
0x146e530 _Z18internal_error_locPKciS0_z                                                       
        sources/gdb/gdbsupport/errors.cc:58                                     
0xcd0ae2 _Z14malloc_failurel           
        sources/gdb/gdb/utils.c:685                                             
0x1474ec6 _Znwm                                
        sources/gdb/gdbsupport/new-op.cc:70
0x41f385 _ZN9__gnu_cxx13new_allocatorIhE8allocateEmPKv                                         
        /usr/include/c++/8/ext/new_allocator.h:111                                             
0x41ed47 _ZNSt16allocator_traitsIN3gdb22default_init_allocatorIhSaIhEEEE8allocateERS3_m
        /usr/include/c++/8/bits/alloc_traits.h:301                                             
0x41e2a5 _ZNSt12_Vector_baseIhN3gdb22default_init_allocatorIhSaIhEEEE11_M_allocateEm           
        /usr/include/c++/8/bits/stl_vector.h:296                                               
0x41d7c2 ???    
0x41c88e _ZNSt12_Vector_baseIhN3gdb22default_init_allocatorIhSaIhEEEEC2EmRKS3_                 
        /usr/include/c++/8/bits/stl_vector.h:260
        /usr/include/c++/8/bits/stl_vector.h:296
0x41d7c2 ???                      
0x41c88e _ZNSt12_Vector_baseIhN3gdb22default_init_allocatorIhSaIhEEEEC2EmRKS3_
        /usr/include/c++/8/bits/stl_vector.h:260                             
0x41b54b ???                                                                                   
0xb79058 _Z23gdb_bfd_scan_elf_dyntagiP3bfdPmS1_      
        sources/gdb/gdb/solib.c:1549 
0xb67d0a elf_locate_base                                                                       
        sources/gdb/gdb/solib-svr4.c:706
0xb6d7d5 svr4_iterate_over_objfiles_in_search_order                                            
        sources/gdb/gdb/solib-svr4.c:3327                                       
0x4e02b1 _Z45gdbarch_iterate_over_objfiles_in_search_orderP7gdbarchN3gdb13function_viewIFbP7objfileEEES4_                                                                                     
        sources/gdb/gdb/gdbarch.c:5078
0xbe3c20 find_main_name                        
        sources/gdb/gdb/symtab.c:6299                                           
0xbe3cb9 _Z13main_languagev                                                                    
        sources/gdb/gdb/symtab.c:6342                         
0xbc1527 _Z20set_initial_languagev
        sources/gdb/gdb/symfile.c:1695                                          
0xbc0092 symbol_file_add_main_1                                                                
        sources/gdb/gdb/symfile.c:1207                                          
0xbbfff2 _Z20symbol_file_add_mainPKc10enum_flagsI16symfile_add_flagE                           
        sources/gdb/gdb/symfile.c:1189                                                                                                                                         
0x8edba1 symbol_file_add_main_adapter
        sources/gdb/gdb/main.c:544                                    
0x8edab6 catch_command_errors               
        sources/gdb/gdb/main.c:513                                              
0x8eeb89 captured_main_1
        sources/gdb/gdb/main.c:1207                                             
0x8ef18b captured_main                                                                         
        sources/gdb/gdb/main.c:1314                                             
0x8ef22a _Z8gdb_mainP18captured_main_args                                                      
        sources/gdb/gdb/main.c:1343                                             
0x4136d9 main                                                                                  
        sources/gdb/gdb/gdb.c:39  
...

BFD is already warning as you can see in the log. But mainly because
the binary is "crafted" to trigger this.

I also did some more investigating:
According to the ELF spec, the section size is an uint32_t. The GDB code
is using an int right now, while bfd_section_size is returning an uint32_t.
So GDB should really use an uint32_t as well. Even if GDB is passing it as a
size_t in the end to allocation. I will fix that in a v2.
As well as updating the commit message with this information.
The "negative" section size is not really a thing. Sorry for not realizing that
earlier.

About size validation:
I don't see how we can realistically do that. We don't want to restrict
users that have a big binary and enough system memory.
Nor do I see any indication that this is a real world problem right now,
and that there are users with a big .dynamic section that experience
memory pressure and still want to debug.
To me the state with my patch is good enough. There are warnings
to figure this out easily. And we even get the backtrace now.

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 with a negative .dynamic section size

[Willgerodt, Felix]

> I also did some more investigating:
> According to the ELF spec, the section size is an uint32_t. The GDB code
> is using an int right now, while bfd_section_size is returning an uint32_t.

Slight correction: bfd is returning an bfd_size_type, which is an
unsigned long.

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 with a negative .dynamic section size

[Tom Tromey]

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

> BFD: warning: bin/poc has a section extending past end of file
> Reading symbols from poc...       
                                                                                               
> warning: Loadable section ".dynamic" outside of ELF segments
>   in bin/poc                                                                    
                                                                                               
> warning: Discarding section .dynamic which has a section size (fffffffa40) larger than the file size [in module bin/poc]

> About size validation:
> I don't see how we can realistically do that. We don't want to restrict
> users that have a big binary and enough system memory.
> Nor do I see any indication that this is a real world problem right now,
> and that there are users with a big .dynamic section that experience
> memory pressure and still want to debug.
> To me the state with my patch is good enough. There are warnings
> to figure this out easily. And we even get the backtrace now.

It seems from the above that BFD is doing some simple sanity checks,
like a section cannot be larger than the file.  So maybe gdb could
replicate or reuse these.

Tom

Re: [PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size

[Tom Tromey]

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

Felix> This binary causes a segfault in GDB, because we pass a negative
Felix> value to alloca in gdb_bfd_scan_elf_dyntag.

gdb really should generally not use alloca.  It's a foot gun.

Felix> diff --git a/gdb/solib.c b/gdb/solib.c
Felix> index b9fb911a810..45a5309d199 100644
Felix> --- a/gdb/solib.c
Felix> +++ b/gdb/solib.c
Felix> @@ -1546,7 +1546,8 @@ gdb_bfd_scan_elf_dyntag (const int desired_dyntag, bfd *abfd, CORE_ADDR *ptr,
Felix>    /* Read in .dynamic from the BFD.  We will get the actual value
Felix>       from memory later.  */
Felix>    sect_size = bfd_section_size (sect);
Felix> -  buf = bufstart = (gdb_byte *) alloca (sect_size);
Felix> +  gdb::byte_vector buffer (sect_size);
Felix> +  buf = bufstart = buffer.data ();
Felix>    if (!bfd_get_section_contents (abfd, sect,

Like Keith I wonder if a better patch is possible here, doing more validation.
However your patch is also an improvement.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

RE: [PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size

[Willgerodt, Felix]

> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Montag, 13. November 2023 18:16
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: Keith Seitz <keiths@redhat.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size
> 
> >>>>> Willgerodt, Felix <felix.willgerodt@intel.com> writes:
> 
> > BFD: warning: bin/poc has a section extending past end of file
> > Reading symbols from poc...
> 
> > warning: Loadable section ".dynamic" outside of ELF segments
> >   in bin/poc
> 
> > warning: Discarding section .dynamic which has a section size (fffffffa40) larger
> than the file size [in module bin/poc]
> 
> > About size validation:
> > I don't see how we can realistically do that. We don't want to restrict
> > users that have a big binary and enough system memory.
> > Nor do I see any indication that this is a real world problem right now,
> > and that there are users with a big .dynamic section that experience
> > memory pressure and still want to debug.
> > To me the state with my patch is good enough. There are warnings
> > to figure this out easily. And we even get the backtrace now.
> 
> It seems from the above that BFD is doing some simple sanity checks,
> like a section cannot be larger than the file.  So maybe gdb could
> replicate or reuse these.

BFD is checking the section size against the file size two times and once
It checks if the section size is staying in the ELF segments region.
I don't think another warning from GDB would help.  Or what GDB at this
part of the code could do differently if it encounters such a thing.

I saw your other email where you approved the patch.  Thanks for that.
I have made one more change and rewrote the commit msg, as it had some
confusing or even wrong wording.  So I will send out a v2 soon.

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 with a negative .dynamic section size

[Tom Tromey]

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

> BFD is checking the section size against the file size two times and once
> It checks if the section size is staying in the ELF segments region.
> I don't think another warning from GDB would help.  Or what GDB at this
> part of the code could do differently if it encounters such a thing.

Ok, I understand the problem now, I think.

bfd_get_section_contents does the size check -- but the buffer for the
contents must be allocated before this call.  So, this approach is
forced on us by BFD's API.

Maybe changing that would be good, but meh, (1) changing BFD can be
difficult, and (2) it seems like a lot of work for fuzzer input.

I do wonder now why gdb generally uses bfd_get_section_contents and not
bfd_get_full_section_contents, as the latter supports decompression while 
former does not.

Tom

RE: [PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size

[Willgerodt, Felix]

> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Dienstag, 14. November 2023 17:30
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: Tom Tromey <tom@tromey.com>; Keith Seitz <keiths@redhat.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb: Fix segfault with a negative .dynamic section size
> 
> >>>>> Willgerodt, Felix <felix.willgerodt@intel.com> writes:
> 
> > BFD is checking the section size against the file size two times and once
> > It checks if the section size is staying in the ELF segments region.
> > I don't think another warning from GDB would help.  Or what GDB at this
> > part of the code could do differently if it encounters such a thing.
> 
> Ok, I understand the problem now, I think.
> 
> bfd_get_section_contents does the size check -- but the buffer for the
> contents must be allocated before this call.  So, this approach is
> forced on us by BFD's API.
> 
> Maybe changing that would be good, but meh, (1) changing BFD can be
> difficult, and (2) it seems like a lot of work for fuzzer input.
> 
> I do wonder now why gdb generally uses bfd_get_section_contents and not
> bfd_get_full_section_contents, as the latter supports decompression while
> former does not.

I am afraid I am not familiar enough with section compression to answer that.
Though I would see it as a separate bug if we don't decompress it but should.
Granted, then we could think about using something like
bfd_malloc_and_get_section and a free().

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 with a negative .dynamic section size

[Tom Tromey]

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

>> I do wonder now why gdb generally uses bfd_get_section_contents and not
>> bfd_get_full_section_contents, as the latter supports decompression while
>> former does not.

> I am afraid I am not familiar enough with section compression to answer that.
> Though I would see it as a separate bug if we don't decompress it but should.

Yeah, me too.
Anyway I was just speculating, I think your approach is fine.

Tom