diff mbox series

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

Message ID 20230601104540.156137-1-lancelot.six@amd.com
State New
Headers
Series [v2,1/3] gdb/corelow.c: fix use-after-free in build_file_mappings |

Commit Message

Lancelot SIX June 1, 2023, 10:45 a.m. UTC 
  Hi,

Thanks John and Andrew for the reviews and suggestions.  Here is a V2
for patch #1 (I have added Andrew's Reviewed-By tag to patch 2 and 3).

John, should I add your Reviewed-By tag in patches 2 and 3 as you
reviewed them as well?

Changes since V1:
 - Only register the bfd in bfd_map if it got successfully opened,
   following John's suggestion.  I have not used the exact pattern
   suggested in the review to avoid duplicating the warning message.

Best,
Lancelot.

---

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 the bfd is only saved in the bfd_map if it
got opened successfully.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

John Baldwin June 1, 2023, 5:05 p.m. UTC | #1 
On 6/1/23 3:45 AM, Lancelot SIX wrote:
> Hi,
> 
> Thanks John and Andrew for the reviews and suggestions.  Here is a V2
> for patch #1 (I have added Andrew's Reviewed-By tag to patch 2 and 3).
> 
> John, should I add your Reviewed-By tag in patches 2 and 3 as you
> reviewed them as well?

Sure, all three LGTM.  Thanks!

Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Andrew Burgess June 7, 2023, 2:54 p.m. UTC | #2 
Lancelot SIX <lancelot.six@amd.com> writes:

> Hi,
>
> Thanks John and Andrew for the reviews and suggestions.  Here is a V2
> for patch #1 (I have added Andrew's Reviewed-By tag to patch 2 and 3).
>
> John, should I add your Reviewed-By tag in patches 2 and 3 as you
> reviewed them as well?
>
> Changes since V1:
>  - Only register the bfd in bfd_map if it got successfully opened,
>    following John's suggestion.  I have not used the exact pattern
>    suggested in the review to avoid duplicating the warning message.
>
> Best,
> Lancelot.
>
> ---
>
> 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 the bfd is only saved in the bfd_map if it
> got opened successfully.
>
> 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.

All 3 patches LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> ---
>  gdb/corelow.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index db489b4280e..54def4198bc 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -258,8 +258,7 @@ core_target::build_file_mappings ()
>  		return;
>  	      }
>  
> -	    bfd = bfd_map[filename] = bfd_openr (expanded_fname.get (),
> -						 "binary");
> +	    bfd = bfd_openr (expanded_fname.get (), "binary");
>  
>  	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
>  	      {
> @@ -284,6 +283,7 @@ core_target::build_file_mappings ()
>  	       This can be checked before/after a core file detach via
>  	       "maint info bfds".  */
>  	    gdb_bfd_record_inclusion (core_bfd, bfd);
> +	    bfd_map[filename] = bfd;
>  	  }
>  
>  	/* Make new BFD section.  All sections have the same name,
> -- 
> 2.34.1
Lancelot SIX June 8, 2023, 1:22 p.m. UTC | #3 
> 
> All 3 patches LGTM.
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>
> 
> Thanks,
> Andrew
> 

Thanks Andrew and John.

I have added the git trailers to the 3 patches and pushed them.

Best,
Lancelot.
diff mbox series

Patch

diff --git a/gdb/corelow.c b/gdb/corelow.c
index db489b4280e..54def4198bc 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -258,8 +258,7 @@  core_target::build_file_mappings ()
 		return;
 	      }
 
-	    bfd = bfd_map[filename] = bfd_openr (expanded_fname.get (),
-						 "binary");
+	    bfd = bfd_openr (expanded_fname.get (), "binary");
 
 	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
 	      {
@@ -284,6 +283,7 @@  core_target::build_file_mappings ()
 	       This can be checked before/after a core file detach via
 	       "maint info bfds".  */
 	    gdb_bfd_record_inclusion (core_bfd, bfd);
+	    bfd_map[filename] = bfd;
 	  }
 
 	/* Make new BFD section.  All sections have the same name,