diff mbox series

[1/3] gdb/corelow.c: fix use-after-free in build_file_mappings

Message ID 20230531160406.3932028-2-lancelot.six@amd.com
State New
Headers
Series Fix use-after-free in gdb/corelow.c + cleanups |

Commit Message

Lancelot SIX May 31, 2023, 4:04 p.m. UTC 
  In core_target::build_file_mappings, GDB tries to open files referenced
in the core dump.

The process goes like this:

    struct bfd *bfd = bfd_map[filename];
    if (bfd == nullptr)
      {
        bfd = bfd_map[filename]
          = bfd_openr (expanded_fname.get (), "binary");
        if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
          {
            if (bfd != nullptr)
              bfd_close (bfd);
            return;
          }
      }
    asection *sec = bfd_make_section_anyway (bfd, "load");
    ...

The problem is that if bfd_check_format fails, we close the bfd but keep
a reference to it in the bfd_map.

If the same filename appears another time in the NT_FILE note, we enter
this code again.  The second time, bfd_map[filename] is not nullptr and
we try to call bfd_make_section_anyway on an already closed BFD, which
is a use-after-free error.

This patch makes sure that after closing the bfd, it is removed from the
bfd_map.

This error got exposed by a recent change in BFD (014a602b86f "Don't
optimise bfd_seek to same position").  Since this change, opening a
coredump which contains mapping to some special files such as a DRI
render node (/dev/dri/renderD$NUM) exposes the issue.  This happens for
example for processes using AMDGPU devices to offload compute tasks.
---
 gdb/corelow.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

John Baldwin May 31, 2023, 6:30 p.m. UTC | #1 
On 5/31/23 9:04 AM, Lancelot SIX via Gdb-patches wrote:
> In core_target::build_file_mappings, GDB tries to open files referenced
> in the core dump.
> 
> The process goes like this:
> 
>      struct bfd *bfd = bfd_map[filename];
>      if (bfd == nullptr)
>        {
>          bfd = bfd_map[filename]
>            = bfd_openr (expanded_fname.get (), "binary");
>          if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
>            {
>              if (bfd != nullptr)
>                bfd_close (bfd);
>              return;
>            }
>        }
>      asection *sec = bfd_make_section_anyway (bfd, "load");
>      ...
> 
> The problem is that if bfd_check_format fails, we close the bfd but keep
> a reference to it in the bfd_map.
> 
> If the same filename appears another time in the NT_FILE note, we enter
> this code again.  The second time, bfd_map[filename] is not nullptr and
> we try to call bfd_make_section_anyway on an already closed BFD, which
> is a use-after-free error.
> 
> This patch makes sure that after closing the bfd, it is removed from the
> bfd_map.
> 
> This error got exposed by a recent change in BFD (014a602b86f "Don't
> optimise bfd_seek to same position").  Since this change, opening a
> coredump which contains mapping to some special files such as a DRI
> render node (/dev/dri/renderD$NUM) exposes the issue.  This happens for
> example for processes using AMDGPU devices to offload compute tasks.
> ---
>   gdb/corelow.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index db489b4280e..77fc4453f94 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -277,7 +277,10 @@ core_target::build_file_mappings ()
>   			   "during file-backed mapping note processing"),
>   			 filename, expanded_fname.get ());
>   		if (bfd != nullptr)
> -		  bfd_close (bfd);
> +		  {
> +		    bfd_close (bfd);
> +		    bfd_map.erase (filename);
> +		  }
>   		return;
>   	      }
>   	    /* Ensure that the bfd will be closed when core_bfd is closed.

Maybe only insert the bfd into the map on success instead of having to do the
erase, that is something like:

   if (bfd == nullptr)
     {
       bfd = bfd_openr (...);
       if (bfd == nullptr)
          return;
       if (!bfd_check_format (...))
         {
           bfd_close (bfd);
           return;
         }
       bfd_map[filename] = bfd;
     }
Andrew Burgess June 1, 2023, 9:57 a.m. UTC | #2 
John Baldwin <jhb@FreeBSD.org> writes:

> On 5/31/23 9:04 AM, Lancelot SIX via Gdb-patches wrote:
>> In core_target::build_file_mappings, GDB tries to open files referenced
>> in the core dump.
>> 
>> The process goes like this:
>> 
>>      struct bfd *bfd = bfd_map[filename];
>>      if (bfd == nullptr)
>>        {
>>          bfd = bfd_map[filename]
>>            = bfd_openr (expanded_fname.get (), "binary");
>>          if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
>>            {
>>              if (bfd != nullptr)
>>                bfd_close (bfd);
>>              return;
>>            }
>>        }
>>      asection *sec = bfd_make_section_anyway (bfd, "load");
>>      ...
>> 
>> The problem is that if bfd_check_format fails, we close the bfd but keep
>> a reference to it in the bfd_map.
>> 
>> If the same filename appears another time in the NT_FILE note, we enter
>> this code again.  The second time, bfd_map[filename] is not nullptr and
>> we try to call bfd_make_section_anyway on an already closed BFD, which
>> is a use-after-free error.
>> 
>> This patch makes sure that after closing the bfd, it is removed from the
>> bfd_map.
>> 
>> This error got exposed by a recent change in BFD (014a602b86f "Don't
>> optimise bfd_seek to same position").  Since this change, opening a
>> coredump which contains mapping to some special files such as a DRI
>> render node (/dev/dri/renderD$NUM) exposes the issue.  This happens for
>> example for processes using AMDGPU devices to offload compute tasks.
>> ---
>>   gdb/corelow.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index db489b4280e..77fc4453f94 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -277,7 +277,10 @@ core_target::build_file_mappings ()
>>   			   "during file-backed mapping note processing"),
>>   			 filename, expanded_fname.get ());
>>   		if (bfd != nullptr)
>> -		  bfd_close (bfd);
>> +		  {
>> +		    bfd_close (bfd);
>> +		    bfd_map.erase (filename);
>> +		  }
>>   		return;
>>   	      }
>>   	    /* Ensure that the bfd will be closed when core_bfd is closed.
>
> Maybe only insert the bfd into the map on success instead of having to do the
> erase, that is something like:
>
>    if (bfd == nullptr)
>      {
>        bfd = bfd_openr (...);
>        if (bfd == nullptr)
>           return;
>        if (!bfd_check_format (...))
>          {
>            bfd_close (bfd);
>            return;
>          }
>        bfd_map[filename] = bfd;
>      }

+1 for this approach.

Thanks,
Andrew


>
> -- 
> John Baldwin
diff mbox series

Patch

diff --git a/gdb/corelow.c b/gdb/corelow.c
index db489b4280e..77fc4453f94 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -277,7 +277,10 @@  core_target::build_file_mappings ()
 			   "during file-backed mapping note processing"),
 			 filename, expanded_fname.get ());
 		if (bfd != nullptr)
-		  bfd_close (bfd);
+		  {
+		    bfd_close (bfd);
+		    bfd_map.erase (filename);
+		  }
 		return;
 	      }
 	    /* Ensure that the bfd will be closed when core_bfd is closed.