| Message ID | 20180511115228.22098-1-andrew.burgess@embecosm.com |
|---|---|
| State | New |
| Headers | show |
On 05/11/2018 12:52 PM, Andrew Burgess wrote: > In commit: > > commit 8ee22052f690c007556b97eed59f49350ece5ca9 > Author: Andrew Burgess <andrew.burgess@embecosm.com> > Date: Thu May 3 17:46:14 2018 +0100 > > gdb/x86: Handle kernels using compact xsave format > > in two places FXSAVE_ADDR was used instead of FXSAVE_MXCSR_ADDR to get > the address of the mxcsr register within the xsave buffer. This will > mean we are potentially accessing the wrong location within the xsave > buffer. > > There are no tests included with this patch. The first mistake would > only trigger an issue if/when the user tries to manually set the mxcsr > register to a value that matches the random (value off stack) value > that is in the xsave buffer, in this case the change by the user will > go unnoticed by GDB, and the default value of mxcsr will be preserved. > > The second mistake only happens on the code path where all x87 > registers are being written out of the register cache. I'm not sure > how to trigger that code path. > OK as is. How did you notice this? Valgrind? Thanks, Pedro Alves
* Pedro Alves <palves@redhat.com> [2018-05-11 18:14:20 +0100]: > On 05/11/2018 12:52 PM, Andrew Burgess wrote: > > In commit: > > > > commit 8ee22052f690c007556b97eed59f49350ece5ca9 > > Author: Andrew Burgess <andrew.burgess@embecosm.com> > > Date: Thu May 3 17:46:14 2018 +0100 > > > > gdb/x86: Handle kernels using compact xsave format > > > > in two places FXSAVE_ADDR was used instead of FXSAVE_MXCSR_ADDR to get > > the address of the mxcsr register within the xsave buffer. This will > > mean we are potentially accessing the wrong location within the xsave > > buffer. > > > > There are no tests included with this patch. The first mistake would > > only trigger an issue if/when the user tries to manually set the mxcsr > > register to a value that matches the random (value off stack) value > > that is in the xsave buffer, in this case the change by the user will > > go unnoticed by GDB, and the default value of mxcsr will be preserved. > > > > The second mistake only happens on the code path where all x87 > > registers are being written out of the register cache. I'm not sure > > how to trigger that code path. > > > > OK as is. > > How did you notice this? Valgrind? That's a funny story... I had reason to compile HEAD on a machine with GCC 5.4, and the build failed with a warning that 'i' was used uninitialised in this block: /* The mxcsr register is written into the xsave buffer if either AVX or SSE is enabled, so only clear it if both of those features require clearing. */ if ((clear_bv & (X86_XSTATE_AVX | X86_XSTATE_SSE)) == (X86_XSTATE_AVX | X86_XSTATE_SSE)) store_unsigned_integer (FXSAVE_ADDR (tdep, regs, i), 2, byte_order, I387_MXCSR_INIT_VAL); Now the warning from GCC is bogus, 'i' will be initialised, but it's not going to be initialised correctly, and when I looked into how to set 'i' to the correct value I realised the mistake I made. After that I double checked all the accesses to mxcsr that I changed, and found the second bug. Just a lucky find at the end of the day :) Thanks, Andrew
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c index aca70c186f9..3effc35b174 100644 --- a/gdb/i387-tdep.c +++ b/gdb/i387-tdep.c @@ -1490,7 +1490,7 @@ i387_collect_xsave (const struct regcache *regcache, int regnum, require clearing. */ if ((clear_bv & (X86_XSTATE_AVX | X86_XSTATE_SSE)) == (X86_XSTATE_AVX | X86_XSTATE_SSE)) - store_unsigned_integer (FXSAVE_ADDR (tdep, regs, i), 2, byte_order, + store_unsigned_integer (FXSAVE_MXCSR_ADDR (regs), 2, byte_order, I387_MXCSR_INIT_VAL); if ((clear_bv & X86_XSTATE_X87)) @@ -1643,7 +1643,7 @@ i387_collect_xsave (const struct regcache *regcache, int regnum, { i = I387_MXCSR_REGNUM (tdep); regcache_raw_collect (regcache, i, raw); - p = FXSAVE_ADDR (tdep, regs, i); + p = FXSAVE_MXCSR_ADDR (regs); if (memcmp (raw, p, 4)) { /* Now, we need to mark one of either SSE of AVX as enabled.
In commit: commit 8ee22052f690c007556b97eed59f49350ece5ca9 Author: Andrew Burgess <andrew.burgess@embecosm.com> Date: Thu May 3 17:46:14 2018 +0100 gdb/x86: Handle kernels using compact xsave format in two places FXSAVE_ADDR was used instead of FXSAVE_MXCSR_ADDR to get the address of the mxcsr register within the xsave buffer. This will mean we are potentially accessing the wrong location within the xsave buffer. There are no tests included with this patch. The first mistake would only trigger an issue if/when the user tries to manually set the mxcsr register to a value that matches the random (value off stack) value that is in the xsave buffer, in this case the change by the user will go unnoticed by GDB, and the default value of mxcsr will be preserved. The second mistake only happens on the code path where all x87 registers are being written out of the register cache. I'm not sure how to trigger that code path. gdb/ChangeLog: * i387-tdep.c (i387_collect_xsave): Use FXSAVE_MXCSR_ADDR not FXSAVE_ADDR for the mxcsr register. --- gdb/ChangeLog | 5 +++++ gdb/i387-tdep.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-)