[PATCH 0/3] Fix use-after-free in gdb/corelow.c + cleanups

[PATCH 0/3] Fix use-after-free in gdb/corelow.c + cleanups

[Lancelot SIX]

Hi,

Since a recent change if BFD (014a602b86f "Don't optimise bfd_seek to
same position"), I started to see ASAN report a use-after-free error
when opening some coredumps.

If the original process had some file mapped in its address space that
GDB can open, but calling bfd_check_format on this file fails, GDB would
close the BFD but keep a pointer to it for later use, leading to
use-after-free.

Such scenario can be seen when the original process had some IO pages
mapped from a DRI render node (/dev/dri/renderD$NUM) as it is the case
when offloading compute tasks to AMDGPU devices.

The first patch in this series fixes the use-after-free error.

Once this issue fixed, GDB does show a warning message once for each
region in the process address space where the special file was mapped.
This is un-necessarily noisy, and does not match what is done when GDB
does not find the file to open (exec_find_file returns null).  The
second patch of the series ensures that the warning message can only be
printed once per file.

Finally, the third patch in this series ensures that GDB does not try to
open a file if it has already failed to open it.

Since I am not sure how I can write a simple test to exercise for this
failure, I have not included one.  I have tested this series on a system
using an AMDGPU device, where I originally encountered the problem.

Lancelot SIX (3):
  gdb/corelow.c: fix use-after-free in build_file_mappings
  gdb/corelow.c: avoid repeated warnings in build_file_mappings
  gdb/corelow.c: do not try to reopen a file if open failed once

 gdb/corelow.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)


base-commit: a15891aaea006d06066573449efbda353dd2863e
-- 
2.34.1

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

[Lancelot SIX]

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. 
-- 
2.34.1

[PATCH 2/3] gdb/corelow.c: avoid repeated warnings in build_file_mappings

[Lancelot SIX]

When GDB opens a coredump it tries to locate and then open all files
which were mapped in the process.

If a file is found but cannot be opened with BFD (bfd_open /
bfd_check_format fails), then a warning is printed to the user.  If the
same file was mapped multiple times in the process's address space, the
warning is printed once for each time the file was mapped.  I find this
un-necessarily noisy.

This patch makes it so the warning message is printed only once per
file.

There was a comment in the code assuming that if the file was found on
the system, opening it (bfd_open + bfd_check_format) should always
succeed.  A recent change in BFD (014a602b86f "Don't optimise bfd_seek
to same position") showed that this assumption is not valid.  For
example, it is possible to have a core dump of a process which had
mmaped an IO page from a DRI render node (/dev/dri/runderD$NUM).  In
such case the core dump does contain the information that portions of
this special file were mapped in the host process, but trying to seek to
position 0 will fail, making bfd_check_format fail.  This patch removes
this comment.
---
 gdb/corelow.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 77fc4453f94..ce68f91132e 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -264,18 +264,11 @@ core_target::build_file_mappings ()
 	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
 	      {
 		m_core_unavailable_mappings.emplace_back (start, end - start);
-		/* If we get here, there's a good chance that it's due to
-		   an internal error.  We issue a warning instead of an
-		   internal error because of the possibility that the
-		   file was removed in between checking for its
-		   existence during the expansion in exec_file_find()
-		   and the calls to bfd_openr() / bfd_check_format(). 
-		   Output both the path from the core file note along
-		   with its expansion to make debugging this problem
-		   easier.  */
-		warning (_("Can't open file %s which was expanded to %s "
-			   "during file-backed mapping note processing"),
-			 filename, expanded_fname.get ());
+		if (unavailable_paths.insert (filename).second)
+		  warning (_("Can't open file %s which was expanded to %s "
+			     "during file-backed mapping note processing"),
+			   filename, expanded_fname.get ());
+
 		if (bfd != nullptr)
 		  {
 		    bfd_close (bfd);
-- 
2.34.1

[PATCH 3/3] gdb/corelow.c: do not try to reopen a file if open failed once

[Lancelot SIX]

In the current implementation, core_target::build_file_mappings will try
to locate and open files which were mapped in the process for which the
core dump was produced.  If the file cannot be found or cannot be
opened, GDB will re-try to open it once for each time it was mapped in
the process's address space.

This patch makes it so GDB recognizes that it has already failed to open
a given file once and does not re-try the process for each mapping.
---
 gdb/corelow.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index ce68f91132e..b831a90792c 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -233,6 +233,16 @@ core_target::build_file_mappings ()
 	   weed out non-file-backed mappings.  */
 	gdb_assert (filename != nullptr);
 
+	if (unavailable_paths.find (filename) != unavailable_paths.end ())
+	  {
+	    /* We have already seen some mapping for FILENAME but failed to
+	       find/open the file.  There is no point in trying the same
+	       thing again so just record that the range [start, end) is
+	       unavailable.  */
+	    m_core_unavailable_mappings.emplace_back (start, end - start);
+	    return;
+	  }
+
 	struct bfd *bfd = bfd_map[filename];
 	if (bfd == nullptr)
 	  {
@@ -250,11 +260,10 @@ core_target::build_file_mappings ()
 	    if (expanded_fname == nullptr)
 	      {
 		m_core_unavailable_mappings.emplace_back (start, end - start);
-		/* Print just one warning per path.  */
-		if (unavailable_paths.insert (filename).second)
-		  warning (_("Can't open file %s during file-backed mapping "
-			     "note processing"),
-			   filename);
+		unavailable_paths.insert (filename);
+		warning (_("Can't open file %s during file-backed mapping "
+			   "note processing"),
+			 filename);
 		return;
 	      }
 
@@ -264,10 +273,10 @@ core_target::build_file_mappings ()
 	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
 	      {
 		m_core_unavailable_mappings.emplace_back (start, end - start);
-		if (unavailable_paths.insert (filename).second)
-		  warning (_("Can't open file %s which was expanded to %s "
-			     "during file-backed mapping note processing"),
-			   filename, expanded_fname.get ());
+		unavailable_paths.insert (filename);
+		warning (_("Can't open file %s which was expanded to %s "
+			   "during file-backed mapping note processing"),
+			 filename, expanded_fname.get ());
 
 		if (bfd != nullptr)
 		  {
-- 
2.34.1

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

[John Baldwin]

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

Re: [PATCH 0/3] Fix use-after-free in gdb/corelow.c + cleanups

[John Baldwin]

On 5/31/23 9:04 AM, Lancelot SIX via Gdb-patches wrote:
> Hi,
> 
> Since a recent change if BFD (014a602b86f "Don't optimise bfd_seek to
> same position"), I started to see ASAN report a use-after-free error
> when opening some coredumps.
> 
> If the original process had some file mapped in its address space that
> GDB can open, but calling bfd_check_format on this file fails, GDB would
> close the BFD but keep a pointer to it for later use, leading to
> use-after-free.
> 
> Such scenario can be seen when the original process had some IO pages
> mapped from a DRI render node (/dev/dri/renderD$NUM) as it is the case
> when offloading compute tasks to AMDGPU devices.
> 
> The first patch in this series fixes the use-after-free error.
> 
> Once this issue fixed, GDB does show a warning message once for each
> region in the process address space where the special file was mapped.
> This is un-necessarily noisy, and does not match what is done when GDB
> does not find the file to open (exec_find_file returns null).  The
> second patch of the series ensures that the warning message can only be
> printed once per file.
> 
> Finally, the third patch in this series ensures that GDB does not try to
> open a file if it has already failed to open it.
> 
> Since I am not sure how I can write a simple test to exercise for this
> failure, I have not included one.  I have tested this series on a system
> using an AMDGPU device, where I originally encountered the problem.
> 
> Lancelot SIX (3):
>    gdb/corelow.c: fix use-after-free in build_file_mappings
>    gdb/corelow.c: avoid repeated warnings in build_file_mappings
>    gdb/corelow.c: do not try to reopen a file if open failed once
> 
>   gdb/corelow.c | 35 ++++++++++++++++++++---------------
>   1 file changed, 20 insertions(+), 15 deletions(-)
> 
> 
> base-commit: a15891aaea006d06066573449efbda353dd2863e

Patches 2 and 3 look good to me.

-- 
John Baldwin

Re: [PATCH 2/3] gdb/corelow.c: avoid repeated warnings in build_file_mappings

[Andrew Burgess]

Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

> When GDB opens a coredump it tries to locate and then open all files
> which were mapped in the process.
>
> If a file is found but cannot be opened with BFD (bfd_open /
> bfd_check_format fails), then a warning is printed to the user.  If the
> same file was mapped multiple times in the process's address space, the
> warning is printed once for each time the file was mapped.  I find this
> un-necessarily noisy.
>
> This patch makes it so the warning message is printed only once per
> file.
>
> There was a comment in the code assuming that if the file was found on
> the system, opening it (bfd_open + bfd_check_format) should always
> succeed.  A recent change in BFD (014a602b86f "Don't optimise bfd_seek
> to same position") showed that this assumption is not valid.  For
> example, it is possible to have a core dump of a process which had
> mmaped an IO page from a DRI render node (/dev/dri/runderD$NUM).  In
> such case the core dump does contain the information that portions of
> this special file were mapped in the host process, but trying to seek to
> position 0 will fail, making bfd_check_format fail.  This patch removes
> this comment.

LGTM.

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

Thanks,
Andrew

> ---
>  gdb/corelow.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 77fc4453f94..ce68f91132e 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -264,18 +264,11 @@ core_target::build_file_mappings ()
>  	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
>  	      {
>  		m_core_unavailable_mappings.emplace_back (start, end - start);
> -		/* If we get here, there's a good chance that it's due to
> -		   an internal error.  We issue a warning instead of an
> -		   internal error because of the possibility that the
> -		   file was removed in between checking for its
> -		   existence during the expansion in exec_file_find()
> -		   and the calls to bfd_openr() / bfd_check_format(). 
> -		   Output both the path from the core file note along
> -		   with its expansion to make debugging this problem
> -		   easier.  */
> -		warning (_("Can't open file %s which was expanded to %s "
> -			   "during file-backed mapping note processing"),
> -			 filename, expanded_fname.get ());
> +		if (unavailable_paths.insert (filename).second)
> +		  warning (_("Can't open file %s which was expanded to %s "
> +			     "during file-backed mapping note processing"),
> +			   filename, expanded_fname.get ());
> +
>  		if (bfd != nullptr)
>  		  {
>  		    bfd_close (bfd);
> -- 
> 2.34.1

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

[Andrew Burgess]

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

Re: [PATCH 3/3] gdb/corelow.c: do not try to reopen a file if open failed once

[Andrew Burgess]

Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

> In the current implementation, core_target::build_file_mappings will try
> to locate and open files which were mapped in the process for which the
> core dump was produced.  If the file cannot be found or cannot be
> opened, GDB will re-try to open it once for each time it was mapped in
> the process's address space.
>
> This patch makes it so GDB recognizes that it has already failed to open
> a given file once and does not re-try the process for each mapping.

LGTM.

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

Thanks,
Andrew

> ---
>  gdb/corelow.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index ce68f91132e..b831a90792c 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -233,6 +233,16 @@ core_target::build_file_mappings ()
>  	   weed out non-file-backed mappings.  */
>  	gdb_assert (filename != nullptr);
>  
> +	if (unavailable_paths.find (filename) != unavailable_paths.end ())
> +	  {
> +	    /* We have already seen some mapping for FILENAME but failed to
> +	       find/open the file.  There is no point in trying the same
> +	       thing again so just record that the range [start, end) is
> +	       unavailable.  */
> +	    m_core_unavailable_mappings.emplace_back (start, end - start);
> +	    return;
> +	  }
> +
>  	struct bfd *bfd = bfd_map[filename];
>  	if (bfd == nullptr)
>  	  {
> @@ -250,11 +260,10 @@ core_target::build_file_mappings ()
>  	    if (expanded_fname == nullptr)
>  	      {
>  		m_core_unavailable_mappings.emplace_back (start, end - start);
> -		/* Print just one warning per path.  */
> -		if (unavailable_paths.insert (filename).second)
> -		  warning (_("Can't open file %s during file-backed mapping "
> -			     "note processing"),
> -			   filename);
> +		unavailable_paths.insert (filename);
> +		warning (_("Can't open file %s during file-backed mapping "
> +			   "note processing"),
> +			 filename);
>  		return;
>  	      }
>  
> @@ -264,10 +273,10 @@ core_target::build_file_mappings ()
>  	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
>  	      {
>  		m_core_unavailable_mappings.emplace_back (start, end - start);
> -		if (unavailable_paths.insert (filename).second)
> -		  warning (_("Can't open file %s which was expanded to %s "
> -			     "during file-backed mapping note processing"),
> -			   filename, expanded_fname.get ());
> +		unavailable_paths.insert (filename);
> +		warning (_("Can't open file %s which was expanded to %s "
> +			   "during file-backed mapping note processing"),
> +			 filename, expanded_fname.get ());
>  
>  		if (bfd != nullptr)
>  		  {
> -- 
> 2.34.1

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

[Lancelot SIX]

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(-)

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

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

[John Baldwin]

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>

-- 
John Baldwin

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

[Andrew Burgess]

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

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

[Lancelot SIX]

> 
> 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.