diff mbox

Allocate buffer with proper size in amd64_pseudo_register_{read_value,write}

Message ID 20181021025948.24787-1-simon.marchi@polymtl.ca
State New, archived
Headers

Commit Message

Simon Marchi Oct. 21, 2018, 2:59 a.m. UTC 
  Running "maintenance selftest" on an amd64 build with AddressSanitizer
enabled, I get this:

==18126==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffdf72397c1 at pc 0x7fb5f437b011 bp 0x7ffdf7239740 sp 0x7ffdf7238ee8
WRITE of size 8 at 0x7ffdf72397c1 thread T0
    #0 0x7fb5f437b010 in __interceptor_memcpy /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:737
    #1 0x55a1f899c1b3 in readable_regcache::raw_read(int, unsigned char*) /home/simark/src/binutils-gdb/gdb/regcache.c:530
    #2 0x55a1f7db241b in amd64_pseudo_register_read_value /home/simark/src/binutils-gdb/gdb/amd64-tdep.c:384
    #3 0x55a1f8413a2e in gdbarch_pseudo_register_read_value(gdbarch*, readable_regcache*, int) /home/simark/src/binutils-gdb/gdb/gdbarch.c:1992
    #4 0x55a1f899c9d1 in readable_regcache::cooked_read(int, unsigned char*) /home/simark/src/binutils-gdb/gdb/regcache.c:636
    #5 0x55a1f89a2251 in cooked_read_test /home/simark/src/binutils-gdb/gdb/regcache.c:1649

In amd64_pseudo_register_read_value, when we try to read the al
register, for example, we need to read rax and extract al from it.  We
allocate a buffer of the size of al (1 byte):

  gdb_byte *raw_buf = (gdb_byte *) alloca (register_size (gdbarch, regnum));

but read in it the whole rax value (8 bytes):

  status = regcache->raw_read (gpnum, raw_buf);

Fix it by allocating a buffer correctly sized for the full register from
which the smaller register is extracted.  The
amd64_pseudo_register_write function had the same problem.

gdb/ChangeLog:

	* amd64-tdep.c (amd64_pseudo_register_read_value): Use
	correctly-sized buffer with raw_read.
	(amd64_pseudo_register_write): Use correctly-sized buffer for
	raw_read/raw_write.
---
 gdb/amd64-tdep.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Comments

Kevin Buettner Oct. 21, 2018, 9:43 p.m. UTC | #1 
On Sat, 20 Oct 2018 22:59:48 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> Running "maintenance selftest" on an amd64 build with AddressSanitizer
> enabled, I get this:
> 
> ==18126==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffdf72397c1 at pc 0x7fb5f437b011 bp 0x7ffdf7239740 sp 0x7ffdf7238ee8
> WRITE of size 8 at 0x7ffdf72397c1 thread T0
>     #0 0x7fb5f437b010 in __interceptor_memcpy /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:737
>     #1 0x55a1f899c1b3 in readable_regcache::raw_read(int, unsigned char*) /home/simark/src/binutils-gdb/gdb/regcache.c:530
>     #2 0x55a1f7db241b in amd64_pseudo_register_read_value /home/simark/src/binutils-gdb/gdb/amd64-tdep.c:384
>     #3 0x55a1f8413a2e in gdbarch_pseudo_register_read_value(gdbarch*, readable_regcache*, int) /home/simark/src/binutils-gdb/gdb/gdbarch.c:1992
>     #4 0x55a1f899c9d1 in readable_regcache::cooked_read(int, unsigned char*) /home/simark/src/binutils-gdb/gdb/regcache.c:636
>     #5 0x55a1f89a2251 in cooked_read_test /home/simark/src/binutils-gdb/gdb/regcache.c:1649
> 
> In amd64_pseudo_register_read_value, when we try to read the al
> register, for example, we need to read rax and extract al from it.  We
> allocate a buffer of the size of al (1 byte):
> 
>   gdb_byte *raw_buf = (gdb_byte *) alloca (register_size (gdbarch, regnum));
> 
> but read in it the whole rax value (8 bytes):
> 
>   status = regcache->raw_read (gpnum, raw_buf);
> 
> Fix it by allocating a buffer correctly sized for the full register from
> which the smaller register is extracted.  The
> amd64_pseudo_register_write function had the same problem.
> 
> gdb/ChangeLog:
> 
> 	* amd64-tdep.c (amd64_pseudo_register_read_value): Use
> 	correctly-sized buffer with raw_read.
> 	(amd64_pseudo_register_write): Use correctly-sized buffer for
> 	raw_read/raw_write.

Okay.

Kevin
Simon Marchi Oct. 22, 2018, 2:10 a.m. UTC | #2 
On 2018-10-21 17:43, Kevin Buettner wrote:
> Okay.
> 
> Kevin

Thanks, I pushed it.

Simon
diff mbox

Patch

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 088542d72b49..abf3e4d91904 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -352,16 +352,12 @@  amd64_pseudo_register_read_value (struct gdbarch *gdbarch,
 				  readable_regcache *regcache,
 				  int regnum)
 {
-  gdb_byte *raw_buf = (gdb_byte *) alloca (register_size (gdbarch, regnum));
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
-  enum register_status status;
-  struct value *result_value;
-  gdb_byte *buf;
 
-  result_value = allocate_value (register_type (gdbarch, regnum));
+  value *result_value = allocate_value (register_type (gdbarch, regnum));
   VALUE_LVAL (result_value) = lval_register;
   VALUE_REGNUM (result_value) = regnum;
-  buf = value_contents_raw (result_value);
+  gdb_byte *buf = value_contents_raw (result_value);
 
   if (i386_byte_regnum_p (gdbarch, regnum))
     {
@@ -370,9 +366,11 @@  amd64_pseudo_register_read_value (struct gdbarch *gdbarch,
       /* Extract (always little endian).  */
       if (gpnum >= AMD64_NUM_LOWER_BYTE_REGS)
 	{
+	  gpnum -= AMD64_NUM_LOWER_BYTE_REGS;
+	  gdb_byte raw_buf[register_size (gdbarch, gpnum)];
+
 	  /* Special handling for AH, BH, CH, DH.  */
-	  status = regcache->raw_read (gpnum - AMD64_NUM_LOWER_BYTE_REGS,
-				       raw_buf);
+	  register_status status = regcache->raw_read (gpnum, raw_buf);
 	  if (status == REG_VALID)
 	    memcpy (buf, raw_buf + 1, 1);
 	  else
@@ -381,7 +379,8 @@  amd64_pseudo_register_read_value (struct gdbarch *gdbarch,
 	}
       else
 	{
-	  status = regcache->raw_read (gpnum, raw_buf);
+	  gdb_byte raw_buf[register_size (gdbarch, gpnum)];
+	  register_status status = regcache->raw_read (gpnum, raw_buf);
 	  if (status == REG_VALID)
 	    memcpy (buf, raw_buf, 1);
 	  else
@@ -392,8 +391,9 @@  amd64_pseudo_register_read_value (struct gdbarch *gdbarch,
   else if (i386_dword_regnum_p (gdbarch, regnum))
     {
       int gpnum = regnum - tdep->eax_regnum;
+      gdb_byte raw_buf[register_size (gdbarch, gpnum)];
       /* Extract (always little endian).  */
-      status = regcache->raw_read (gpnum, raw_buf);
+      register_status status = regcache->raw_read (gpnum, raw_buf);
       if (status == REG_VALID)
 	memcpy (buf, raw_buf, 4);
       else
@@ -412,7 +412,6 @@  amd64_pseudo_register_write (struct gdbarch *gdbarch,
 			     struct regcache *regcache,
 			     int regnum, const gdb_byte *buf)
 {
-  gdb_byte *raw_buf = (gdb_byte *) alloca (register_size (gdbarch, regnum));
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   if (i386_byte_regnum_p (gdbarch, regnum))
@@ -421,15 +420,20 @@  amd64_pseudo_register_write (struct gdbarch *gdbarch,
 
       if (gpnum >= AMD64_NUM_LOWER_BYTE_REGS)
 	{
+	  gpnum -= AMD64_NUM_LOWER_BYTE_REGS;
+	  gdb_byte raw_buf[register_size (gdbarch, gpnum)];
+
 	  /* Read ... AH, BH, CH, DH.  */
-	  regcache->raw_read (gpnum - AMD64_NUM_LOWER_BYTE_REGS, raw_buf);
+	  regcache->raw_read (gpnum, raw_buf);
 	  /* ... Modify ... (always little endian).  */
 	  memcpy (raw_buf + 1, buf, 1);
 	  /* ... Write.  */
-	  regcache->raw_write (gpnum - AMD64_NUM_LOWER_BYTE_REGS, raw_buf);
+	  regcache->raw_write (gpnum, raw_buf);
 	}
       else
 	{
+	  gdb_byte raw_buf[register_size (gdbarch, gpnum)];
+
 	  /* Read ...  */
 	  regcache->raw_read (gpnum, raw_buf);
 	  /* ... Modify ... (always little endian).  */
@@ -441,6 +445,7 @@  amd64_pseudo_register_write (struct gdbarch *gdbarch,
   else if (i386_dword_regnum_p (gdbarch, regnum))
     {
       int gpnum = regnum - tdep->eax_regnum;
+      gdb_byte raw_buf[register_size (gdbarch, gpnum)];
 
       /* Read ...  */
       regcache->raw_read (gpnum, raw_buf);