diff mbox

Linux/gdbserver: Correctly handle narrow big-endian register transfers

Message ID alpine.DEB.2.00.1804121514580.9831@tp.orcam.me.uk
State Superseded
Headers show

Commit Message

Maciej W. Rozycki May 22, 2018, 10:30 a.m. UTC
An issue with buffer overruns has been fixed with commit 48d93c7500d9 
("gdbserver fetch/store registers problem on s390x"), 
<https://sourceware.org/ml/gdb-patches/2005-05/msg00224.html>, that 
affected register transfers via the `ptrace' PTRACE_PEEKUSR and 
PTRACE_POKEUSR requests accessing registers or register parts that are 
narrower than the `ptrace' transfer data type.  However that did not 
address the issue where on big-endian systems it's most-significant bits 
of such narrow register data that are appended to or fetched from the 
buffer.

It affects big-endian n64 MIPS targets where $dspctl is 32-bit while the 
`ptrace' transfer data type is 64-bit and consequently the register is 
presented and written as all-zeros held in the most-significant part of 
the data quantity passed:

(gdb) info registers
                  zero               at               v0               v1
 R0   0000000000000000 0000000000000001 0000000000000001 0000000000000000
                    a0               a1               a2               a3
 R4   00000001200212b0 0000000000000000 0000000000000021 000000012001a260
                    a4               a5               a6               a7
 R8   000000012001a260 0000000000000004 800000010c60c000 fffffffffffffff8
                    t0               t1               t2               t3
 R12  0000000000000000 000000fff7edab68 0000000000000001 0000000000000000
                    s0               s1               s2               s3
 R16  000000fff7ee2068 0000000120008b80 0000000000000000 0000000000000000
                    s4               s5               s6               s7
 R20  000000000052e5c8 000000000052f008 0000000000000000 0000000000000000
                    t8               t9               k0               k1
 R24  0000000000000000 00000001200027c0 0000000000000000 0000000000000000
                    gp               sp               s8               ra
 R28  00000001200212b0 000000ffffffc850 000000ffffffc850 0000000120005ee8
                status               lo               hi         badvaddr
      0000000000109cf3 0000000000943efe 000000000000000e 000000012001a008
                 cause               pc
      0000000000800024 0000000120005ee8
                  fcsr              fir              hi1              lo1
              0e800000         00f30000 0000000000000000 0101010101010101
                   hi2              lo2              hi3              lo3
      0202020202020202 0303030303030303 0404040404040404 0505050505050505
                dspctl          restart
              00000000 0000000000000000
(gdb) 

Fix this problem then by shifting the narrow chunk of data on big-endian 
systems such that it occupies the most-significant part at the time it 
is written to or read from the buffer, therefore placing it at the 
beginning of the memory location of the buffer used:

(gdb) info registers
                  zero               at               v0               v1
 R0   0000000000000000 0000000000000001 0000000000000001 0000000000000000
                    a0               a1               a2               a3
 R4   00000001200212b0 0000000000000000 0000000000000021 000000012001a260
                    a4               a5               a6               a7
 R8   000000012001a260 0000000000000004 800000010d82e900 fffffffffffffff8
                    t0               t1               t2               t3
 R12  0000000000000000 000000fff7edab68 0000000000000001 0000000000000000
                    s0               s1               s2               s3
 R16  000000fff7ee2068 0000000120008b80 0000000000000000 0000000000000000
                    s4               s5               s6               s7
 R20  000000000052e5c8 000000000052f008 0000000000000000 0000000000000000
                    t8               t9               k0               k1
 R24  0000000000000000 00000001200027c0 0000000000000000 0000000000000000
                    gp               sp               s8               ra
 R28  00000001200212b0 000000ffffffc850 000000ffffffc850 0000000120005ee8
                status               lo               hi         badvaddr
      0000000000109cf3 0000000000943efe 000000000000000e 000000012001a008
                 cause               pc
      0000000000800024 0000000120005ee8
                  fcsr              fir              hi1              lo1
              0e800000         00f30000 0000000000000000 0101010101010101
                   hi2              lo2              hi3              lo3
      0202020202020202 0303030303030303 0404040404040404 0505050505050505
                dspctl          restart
              55aa33cc 0000000000000000
(gdb) 

	gdb/gdbserver/
	* linux-low.c (fetch_register) [__BYTE_ORDER == __BIG_ENDIAN]: 
	Align partial data quantities with the buffer location pointed.
	(store_register) [__BYTE_ORDER == __BIG_ENDIAN]: Move partial 
	data quantities extracted from the buffer back into their 
	position.
---
Ulrich,

 This fixes the problem for MIPS, however from my understanding of your 
commit it will break s390x unless it is further adjusted in a 
target-dependent manner.  So my question is: does `ptrace' indeed place 
32-bit quantities transferred in bits 63:32 of the 64-bit integer data 
quantity passed?

 If so, then would wrapping this into `#ifndef __s390x__' be OK with you 
as it's s390x that appears to be an oddball here by representing the 
same 32-bit integer data quantity differently between 32-bit and 64-bit 
systems (i.e. you can't just cast `long' to `int')?

 I have no access to an s390x to verify this change.  This has passed 
regression testing with o32 and n64 targets with `gdbserver' strapped to 
use PTRACE_PEEKUSR and PTRACE_POKEUSR requests across all registers.

  Maciej
---
 gdb/gdbserver/linux-low.c |   45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

gdb-gdbserver-linux-fetch-store-register-uneven.diff
diff mbox

Patch

Index: binutils/gdb/gdbserver/linux-low.c
===================================================================
--- binutils.orig/gdb/gdbserver/linux-low.c	2018-05-14 04:01:44.000000000 +0100
+++ binutils/gdb/gdbserver/linux-low.c	2018-05-18 14:48:37.687902646 +0100
@@ -30,6 +30,7 @@ 
 #include "nat/linux-ptrace.h"
 #include "nat/linux-procfs.h"
 #include "nat/linux-personality.h"
+#include <endian.h>
 #include <signal.h>
 #include <sys/ioctl.h>
 #include <fcntl.h>
@@ -5532,6 +5533,7 @@  fetch_register (const struct usrregs_inf
 		struct regcache *regcache, int regno)
 {
   CORE_ADDR regaddr;
+  int bufsize;
   int i, size;
   char *buf;
   int pid;
@@ -5545,20 +5547,29 @@  fetch_register (const struct usrregs_inf
   if (regaddr == -1)
     return;
 
-  size = ((register_size (regcache->tdesc, regno)
-	   + sizeof (PTRACE_XFER_TYPE) - 1)
-	  & -sizeof (PTRACE_XFER_TYPE));
-  buf = (char *) alloca (size);
+  size = register_size (regcache->tdesc, regno);
+  bufsize = ((size + sizeof (PTRACE_XFER_TYPE) - 1)
+	     & -sizeof (PTRACE_XFER_TYPE));
+  buf = (char *) alloca (bufsize);
 
   pid = lwpid_of (current_thread);
   for (i = 0; i < size; i += sizeof (PTRACE_XFER_TYPE))
     {
+      PTRACE_XFER_TYPE val;
+
       errno = 0;
-      *(PTRACE_XFER_TYPE *) (buf + i) =
+      val =
 	ptrace (PTRACE_PEEKUSER, pid,
 		/* Coerce to a uintptr_t first to avoid potential gcc warning
 		   of coercing an 8 byte integer to a 4 byte pointer.  */
 		(PTRACE_TYPE_ARG3) (uintptr_t) regaddr, (PTRACE_TYPE_ARG4) 0);
+#if (__BYTE_ORDER == __BIG_ENDIAN)
+      /* Move the actual data bytes of a partial data quantity into their
+	 positions at the end of the buffer.  */
+      if (size - i < sizeof (PTRACE_XFER_TYPE))
+	val <<= (sizeof (PTRACE_XFER_TYPE) - (size - i)) * 8;
+#endif
+      *(PTRACE_XFER_TYPE *) (buf + i) = val;
       regaddr += sizeof (PTRACE_XFER_TYPE);
       if (errno != 0)
 	{
@@ -5580,6 +5591,7 @@  store_register (const struct usrregs_inf
 		struct regcache *regcache, int regno)
 {
   CORE_ADDR regaddr;
+  int bufsize;
   int i, size;
   char *buf;
   int pid;
@@ -5593,11 +5605,11 @@  store_register (const struct usrregs_inf
   if (regaddr == -1)
     return;
 
-  size = ((register_size (regcache->tdesc, regno)
-	   + sizeof (PTRACE_XFER_TYPE) - 1)
-	  & -sizeof (PTRACE_XFER_TYPE));
-  buf = (char *) alloca (size);
-  memset (buf, 0, size);
+  size = register_size (regcache->tdesc, regno);
+  bufsize = ((size + sizeof (PTRACE_XFER_TYPE) - 1)
+	     & -sizeof (PTRACE_XFER_TYPE));
+  buf = (char *) alloca (bufsize);
+  memset (buf, 0, bufsize);
 
   if (the_low_target.collect_ptrace_register)
     the_low_target.collect_ptrace_register (regcache, regno, buf);
@@ -5607,12 +5619,21 @@  store_register (const struct usrregs_inf
   pid = lwpid_of (current_thread);
   for (i = 0; i < size; i += sizeof (PTRACE_XFER_TYPE))
     {
+      PTRACE_XFER_TYPE val;
+
       errno = 0;
+      val = *(PTRACE_XFER_TYPE *) (buf + i);
+#if (__BYTE_ORDER == __BIG_ENDIAN)
+      /* Move the actual data bytes of a partial data quantity from their
+	 positions at the end of the buffer.  */
+      if (size - i < sizeof (PTRACE_XFER_TYPE))
+	val = (val >> (sizeof (PTRACE_XFER_TYPE) - (size - i)) * 8
+	       & (((PTRACE_XFER_TYPE) 1 << (size - i) * 8) - 1));
+#endif
       ptrace (PTRACE_POKEUSER, pid,
 	    /* Coerce to a uintptr_t first to avoid potential gcc warning
 	       about coercing an 8 byte integer to a 4 byte pointer.  */
-	      (PTRACE_TYPE_ARG3) (uintptr_t) regaddr,
-	      (PTRACE_TYPE_ARG4) *(PTRACE_XFER_TYPE *) (buf + i));
+	      (PTRACE_TYPE_ARG3) (uintptr_t) regaddr, (PTRACE_TYPE_ARG4) val);
       if (errno != 0)
 	{
 	  /* At this point, ESRCH should mean the process is