* [PATCH] Linux/gdbserver: Correctly handle narrow big-endian register transfers
@ 2018-05-22 11:15 Maciej W. Rozycki
2018-05-22 12:48 ` Ulrich Weigand
0 siblings, 1 reply; 3+ messages in thread
From: Maciej W. Rozycki @ 2018-05-22 11:15 UTC (permalink / raw)
To: Ulrich Weigand, gdb-patches; +Cc: Andreas Arnez
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
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Linux/gdbserver: Correctly handle narrow big-endian register transfers
2018-05-22 11:15 [PATCH] Linux/gdbserver: Correctly handle narrow big-endian register transfers Maciej W. Rozycki
@ 2018-05-22 12:48 ` Ulrich Weigand
2018-05-22 22:30 ` [committed] MIPS/gdbserver: " Maciej W. Rozycki
0 siblings, 1 reply; 3+ messages in thread
From: Ulrich Weigand @ 2018-05-22 12:48 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gdb-patches, Andreas Arnez
Maciej W. Rozycki wrote:
> 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.
Well, my understanding of PTRACE_PEEKUSR and PTRACE_POKEUSR is that they
are supposed to simulate access to a "user area", i.e. a data structure
that holds per-process status, in word sized chunks. Now of course the
Linux kernel doesn't actually maintain such a "user area", but for the
purpose of those ptrace calls, it is supposed to pretend it does.
So if we e.g. have some word-sized floating-point registers and a halfword
size status register, the kernel would pretend they are stored in a struct
like this:
struct user_area
{
long fpr[16];
int fpc;
};
and if you access this with PTRACE_PEEKUSR at offset
offsetof(struct user_area, fpc)
you get the 8 bytes at this offset. On a big-endian system, those
correspond to getting the 4 bytes of fpc in the most-significant
bytes, and some padding in the least-significant bytes.
This is what gdbserver expects, and what the kernel ptrace logic
(at least on s390x, I cannot say about others) implements:
} else if (addr == (addr_t) &dummy->regs.fp_regs.fpc) {
/*
* floating point control reg. is in the thread structure
*/
tmp = child->thread.fpu.fpc;
tmp <<= BITS_PER_LONG - 32;
If this is implemented differently on mips, I guess gdbserver will
have to handle this, but I don't really like an #ifdef here.
Can't you move the special logic into platform-specific
the_low_target.collect_ptrace_register and
the_low_target.supply_ptrace_register calls instead?
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
Ulrich.Weigand@de.ibm.com
^ permalink raw reply [flat|nested] 3+ messages in thread
* [committed] MIPS/gdbserver: Correctly handle narrow big-endian register transfers
2018-05-22 12:48 ` Ulrich Weigand
@ 2018-05-22 22:30 ` Maciej W. Rozycki
0 siblings, 0 replies; 3+ messages in thread
From: Maciej W. Rozycki @ 2018-05-22 22:30 UTC (permalink / raw)
To: Ulrich Weigand; +Cc: Andreas Arnez, gdb-patches
Fix an issue with `gdbserver' on big-endian n64 MIPS targets, where
$dspctl is 32-bit while the `ptrace' transfer data type is 64-bit.
Such register data is held in the low order 32 bits of the 64-bit data
quantity exchanged with the buffer used by `fetch_register' and
`store_register', however `supply_register' and `collect_register'
access the same data as a 32-bit quantity. Consequently the register is
presented and written as all-zeros held in the most-significant part of
the big-endian 64-bit data quantity represented in the buffer:
(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)
Correct this problem then by using the `mips_supply_register'
`mips_collect_register' accessors for 32-bit registers where the
`ptrace' data type is 64-bit. These accessors already operate on 32-bit
data quantities held in 64-bit containers:
(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-mips-low.c [HAVE_PTRACE_GETREGS] (mips_collect_register)
(mips_supply_register): Move outside HAVE_PTRACE_GETREGS.
(mips_collect_ptrace_register, mips_supply_ptrace_register): New
functions.
(the_low_target): Wire them.
---
On Tue, 22 May 2018, Ulrich Weigand wrote:
> Maciej W. Rozycki wrote:
>
> > 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?
>
> Well, my understanding of PTRACE_PEEKUSR and PTRACE_POKEUSR is that they
> are supposed to simulate access to a "user area", i.e. a data structure
> that holds per-process status, in word sized chunks. Now of course the
> Linux kernel doesn't actually maintain such a "user area", but for the
> purpose of those ptrace calls, it is supposed to pretend it does.
Now that you write it it makes sense to me and I can only conclude that
maybe it's MIPS that is an oddball here rather than any other target.
It could be IRIX legacy, but MIPS USR addresses are effectively indices,
that is R0 is at 0, R1 is at 1, and so on, and so on, and then finally LO3
is at 76 and DSPControl is at 77, regardless of their actual width.
Partial quantities cannot be accessed, hence there's no way to correctly
read or write say said LO3 with the n32 ABI where the register is 64-bit
and ptrace(2) can only pass 32-bit quantities.
We cannot do anything about it as the MIPS/Linux API has been set since
like 1994. The way to move forward is to switch to the PTRACE_GETREGSET
and PTRACE_SETREGSET API, but for DSP I have proposed the required Linux
kernel part last week only.
> Can't you move the special logic into platform-specific
> the_low_target.collect_ptrace_register and
> the_low_target.supply_ptrace_register calls instead?
Agreed. I only discovered the s390x case as I tracked down the commit
history as I was about to submit the change. I'm so glad that I asked you
directly, and thanks for the quick response.
I guess this also means commit 1d7611244c14 ("PR gdb/22286:
linux-nat-trad: Support arbitrary register widths") might have to be
revisited, although the need doesn't seem immediate as it only affects
MIPS and Alpha I believe, and Alpha does not appear to have been ever used
big-endian (and by the look of arch/alpha/kernel/ptrace.c in Linux it also
uses indices rather than offsets, e.g. R0 is at 0, R1 is at 1, and so on,
hmm...).
So I actually wonder what the distribution of the targets we support is
between offset vs index USR addressing.
Anyway, this replacement change has passed full regression testing with
o32 and n64 targets, with `gdbserver' strapped to use PTRACE_PEEKUSR and
PTRACE_POKEUSR requests across all registers. I have also smoke-tested
n32, verifying that registers show correctly. Committed.
Thank you for your input.
Maciej
---
gdb/gdbserver/linux-mips-low.c | 48 +++++++++++++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 4 deletions(-)
gdb-mips-gdbserver-fetch-store-register-uneven.diff
Index: binutils/gdb/gdbserver/linux-mips-low.c
===================================================================
--- binutils.orig/gdb/gdbserver/linux-mips-low.c 2018-05-22 01:00:12.000000000 +0100
+++ binutils/gdb/gdbserver/linux-mips-low.c 2018-05-22 22:10:11.049273576 +0100
@@ -677,8 +677,6 @@ ps_get_thread_area (struct ps_prochandle
return PS_OK;
}
-#ifdef HAVE_PTRACE_GETREGS
-
static void
mips_collect_register (struct regcache *regcache,
int use_64bit, int regno, union mips_register *reg)
@@ -711,6 +709,8 @@ mips_supply_register (struct regcache *r
supply_register (regcache, regno, reg->buf + offset);
}
+#ifdef HAVE_PTRACE_GETREGS
+
static void
mips_collect_register_32bit (struct regcache *regcache,
int use_64bit, int regno, unsigned char *buf)
@@ -846,6 +846,46 @@ mips_store_fpregset (struct regcache *re
}
#endif /* HAVE_PTRACE_GETREGS */
+/* Take care of 32-bit registers with 64-bit ptrace, POKEUSER side. */
+
+static void
+mips_collect_ptrace_register (struct regcache *regcache,
+ int regno, char *buf)
+{
+ const struct target_desc *tdesc = current_process ()->tdesc;
+ int use_64bit = sizeof (PTRACE_XFER_TYPE) == 8;
+
+ if (use_64bit && register_size (regcache->tdesc, regno) == 4)
+ {
+ union mips_register reg;
+
+ mips_collect_register (regcache, 0, regno, ®);
+ memcpy (buf, ®, sizeof (reg));
+ }
+ else
+ collect_register (regcache, regno, buf);
+}
+
+/* Take care of 32-bit registers with 64-bit ptrace, PEEKUSER side. */
+
+static void
+mips_supply_ptrace_register (struct regcache *regcache,
+ int regno, const char *buf)
+{
+ const struct target_desc *tdesc = current_process ()->tdesc;
+ int use_64bit = sizeof (PTRACE_XFER_TYPE) == 8;
+
+ if (use_64bit && register_size (regcache->tdesc, regno) == 4)
+ {
+ union mips_register reg;
+
+ memcpy (®, buf, sizeof (reg));
+ mips_supply_register (regcache, 0, regno, ®);
+ }
+ else
+ supply_register (regcache, regno, buf);
+}
+
static struct regset_info mips_regsets[] = {
#ifdef HAVE_PTRACE_GETREGS
{ PTRACE_GETREGS, PTRACE_SETREGS, 0, 38 * 8, GENERAL_REGS,
@@ -916,8 +956,8 @@ struct linux_target_ops the_low_target =
mips_remove_point,
mips_stopped_by_watchpoint,
mips_stopped_data_address,
- NULL,
- NULL,
+ mips_collect_ptrace_register,
+ mips_supply_ptrace_register,
NULL, /* siginfo_fixup */
mips_linux_new_process,
mips_linux_delete_process,
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-05-22 21:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-22 11:15 [PATCH] Linux/gdbserver: Correctly handle narrow big-endian register transfers Maciej W. Rozycki
2018-05-22 12:48 ` Ulrich Weigand
2018-05-22 22:30 ` [committed] MIPS/gdbserver: " Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).