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

[Andrew Burgess <aburgess@redhat.com>]

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