Re: [PATCH] gdb/x86: Fix write out of mxcsr register for xsave targets

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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