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

[Keith Seitz <keiths@redhat.com>]

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;