diff mbox series

GDB: Fix out of bounds accesses with limited-length values

Message ID alpine.DEB.2.20.2302231218190.15477@tpp.orcam.me.uk
State Committed
Headers
Series GDB: Fix out of bounds accesses with limited-length values |

Commit Message

Maciej W. Rozycki Feb. 23, 2023, 9:14 p.m. UTC 
  Fix accesses to limited-length values in `contents_copy_raw' and 
`contents_copy_raw_bitwise' so that they observe the limit of the 
original allocation.

Reported by Simon Marchi as a heap-buffer-overflow AddressSanitizer 
issue triggered with gdb.ada/limited-length.exp.
---
Hi,

 Verified to remove the original issue and not to cause any regressions
with and w/o AddressSanitizer and native `x86_64-linux-gnu'.  OK to apply?

  Maciej
---
 gdb/value.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

gdb-limited-length-array-value-contents-copy-fix.diff

Comments

Simon Marchi Feb. 23, 2023, 9:34 p.m. UTC | #1 
On 2/23/23 16:14, Maciej W. Rozycki wrote:
> Fix accesses to limited-length values in `contents_copy_raw' and 
> `contents_copy_raw_bitwise' so that they observe the limit of the 
> original allocation.
> 
> Reported by Simon Marchi as a heap-buffer-overflow AddressSanitizer 
> issue triggered with gdb.ada/limited-length.exp.
> ---
> Hi,
> 
>  Verified to remove the original issue and not to cause any regressions
> with and w/o AddressSanitizer and native `x86_64-linux-gnu'.  OK to apply?
> 
>   Maciej

Thanks, that LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
Maciej W. Rozycki Feb. 24, 2023, 12:39 p.m. UTC | #2 
On Thu, 23 Feb 2023, Simon Marchi wrote:

> > Fix accesses to limited-length values in `contents_copy_raw' and 
> > `contents_copy_raw_bitwise' so that they observe the limit of the 
> > original allocation.
> 
> Thanks, that LGTM:

 Committed, thanks for your review.

  Maciej
diff mbox series

Patch

Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c
+++ src/gdb/value.c
@@ -1168,6 +1168,11 @@  value::contents_copy_raw (struct value *
      mean we'd be copying garbage.  */
   gdb_assert (!dst->m_lazy && !m_lazy);
 
+  ULONGEST copy_length = length;
+  ULONGEST limit = m_limited_length;
+  if (limit > 0 && src_offset + length > limit)
+    copy_length = src_offset > limit ? 0 : limit - src_offset;
+
   /* The overwritten DST range gets unavailability ORed in, not
      replaced.  Make sure to remember to implement replacing if it
      turns out actually necessary.  */
@@ -1178,10 +1183,10 @@  value::contents_copy_raw (struct value *
   /* Copy the data.  */
   gdb::array_view<gdb_byte> dst_contents
     = dst->contents_all_raw ().slice (dst_offset * unit_size,
-					  length * unit_size);
+				      copy_length * unit_size);
   gdb::array_view<const gdb_byte> src_contents
     = contents_all_raw ().slice (src_offset * unit_size,
-				 length * unit_size);
+				 copy_length * unit_size);
   gdb::copy (src_contents, dst_contents);
 
   /* Copy the meta-data, adjusted.  */
@@ -1206,6 +1211,12 @@  value::contents_copy_raw_bitwise (struct
      mean we'd be copying garbage.  */
   gdb_assert (!dst->m_lazy && !m_lazy);
 
+  ULONGEST copy_bit_length = bit_length;
+  ULONGEST bit_limit = m_limited_length * TARGET_CHAR_BIT;
+  if (bit_limit > 0 && src_bit_offset + bit_length > bit_limit)
+    copy_bit_length = (src_bit_offset > bit_limit ? 0
+		       : bit_limit - src_bit_offset);
+
   /* The overwritten DST range gets unavailability ORed in, not
      replaced.  Make sure to remember to implement replacing if it
      turns out actually necessary.  */
@@ -1220,7 +1231,7 @@  value::contents_copy_raw_bitwise (struct
   gdb::array_view<const gdb_byte> src_contents = contents_all_raw ();
   copy_bitwise (dst_contents.data (), dst_bit_offset,
 		src_contents.data (), src_bit_offset,
-		bit_length,
+		copy_bit_length,
 		type_byte_order (type ()) == BFD_ENDIAN_BIG);
 
   /* Copy the meta-data.  */