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

[John Baldwin <jhb@FreeBSD.org>]

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;
     }

-- 
John Baldwin