[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

Ivo Raisr  changed:

   What|Removed |Added

 Status|REOPENED|RESOLVED
 Resolution|--- |FIXED

--- Comment #36 from Ivo Raisr  ---
32-bit tests connected under SVN r16423.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #35 from Ivo Raisr  ---
For Solaris and OS X, tests connected under SVN r16422.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #34 from Ivo Raisr  ---
Follow up SVN commit r16421 which splits the tests into three.
Another commit will follow, enabling cet_nops_fs on Solaris and cet_nops_gs on
OS X.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #33 from Tanya  ---
(In reply to Julian Seward from comment #32)
> (In reply to Tanya from comment #23)
> > Attached an updated 32-bit version of the patch. 32-bit part of the previous
> > patch sets "temp_sorb" variable incorrectly.
> > As far as I understand, the test case should be the same for 32-bit
> > architecture.
> 
> Committed, with some tidying, r3385.  Thanks for the patch.
> 
> Also for the split tests.  Ivo very kindly offered to connect them to
> the build system.

Ivo, Julian,
Thank you very much!

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #32 from Julian Seward  ---
(In reply to Tanya from comment #23)
> Attached an updated 32-bit version of the patch. 32-bit part of the previous
> patch sets "temp_sorb" variable incorrectly.
> As far as I understand, the test case should be the same for 32-bit
> architecture.

Committed, with some tidying, r3385.  Thanks for the patch.

Also for the split tests.  Ivo very kindly offered to connect them to
the build system.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #31 from Julian Seward  ---
(In reply to Tanya from comment #30)
> It should be a plain text file, it opens correctly for me. It does nor open
> for you?

I think the spaces in the file name have confused my emacs :-/  I will
try again.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #30 from Tanya  ---
(In reply to Julian Seward from comment #27)
> (In reply to Tanya from comment #26)
> > Created attachment 105777 [details]
> > test without FS or GS prefixes
> 
> Tanya, is that a tar file?  Can you just attach a plain text file?

It should be a plain text file, it opens correctly for me. It does nor open for
you?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #29 from Tanya  ---
Created attachment 105779
  --> https://bugs.kde.org/attachment.cgi?id=105779=edit
test with FS prefixes

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #27 from Julian Seward  ---
(In reply to Tanya from comment #26)
> Created attachment 105777 [details]
> test without FS or GS prefixes

Tanya, is that a tar file?  Can you just attach a plain text file?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #27 from Julian Seward  ---
(In reply to Tanya from comment #26)
> Created attachment 105777 [details]
> test without FS or GS prefixes

Tanya, is that a tar file?  Can you just attach a plain text file?

--- Comment #28 from Tanya  ---
Created attachment 105778
  --> https://bugs.kde.org/attachment.cgi?id=105778=edit
test with GS prefixes

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

Tanya  changed:

   What|Removed |Added

 Attachment #105776|0   |1
is obsolete||

--- Comment #26 from Tanya  ---
Created attachment 105777
  --> https://bugs.kde.org/attachment.cgi?id=105777=edit
test without FS or GS prefixes

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #25 from Tanya  ---
Created attachment 105776
  --> https://bugs.kde.org/attachment.cgi?id=105776=edit
test without FS or GS prefixes

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

Julian Seward  changed:

   What|Removed |Added

 Status|RESOLVED|REOPENED
 Resolution|FIXED   |---
 Ever confirmed|0   |1

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #24 from Julian Seward  ---
(In reply to Ivo Raisr from comment #18)
> It would help immensely if you could divide the file (or better split it) in
> three sections:
> - with gs prefix
> - with fs prefix
> - with no prefix

Ivo, would it be enough to move the test case from none/tests/amd64 to
none/tests/amd64-linux?  Will that lose us any test-coverage effectiveness?
I think it would be OK.

We could split it into three tests (with gs, with fs, with none) but
that's some work.  Using #ifdefs inside a single file is tricky because
we'd then need to have 3 different .stdout.exp files, which seems a bit
inelegant.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #23 from Tanya  ---
(In reply to Ivo Raisr from comment #19)
> (In reply to Tanya from comment #17)
> > (In reply to Julian Seward from comment #9)
> > > Committed, r3383, r16415.  Thanks for the patches.
> > 
> > Julian,
> > Would it be possible to patch the 32-bit instruction parser as well?
> 
> Sure, no problem. Please provide a patch and test cases.

Ivo, Julian,
Attached an updated 32-bit version of the patch. 32-bit part of the previous
patch sets "temp_sorb" variable incorrectly.
As far as I understand, the test case should be the same for 32-bit
architecture.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #22 from Tanya  ---
Created attachment 105753
  --> https://bugs.kde.org/attachment.cgi?id=105753=edit
Proposed patch for 32-bit architecture

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[H . J . Lu]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #21 from H.J. Lu  ---
(In reply to Julian Seward from comment #20)
> (In reply to Tanya from comment #17)
> > Would it be possible to patch the 32-bit instruction parser as well?
> 
> I saw that part in your patch, but didn't land it, because the 32-bit
> insn parser only goes up to and including SSSE3.  So I thought that it
> didn't need to be patched.
> 
> But now I'm not so sure.  Is there any realistic scenario where we try
> to run 32 bit code on a CPU that advertises (via CPUID) that it can do
> only up until SSSE3, but also contains these NOP instructions?  I think
> that's the key question.

All CPUs which support SSSE3 also support these NOPs.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #20 from Julian Seward  ---
(In reply to Tanya from comment #17)
> Would it be possible to patch the 32-bit instruction parser as well?

I saw that part in your patch, but didn't land it, because the 32-bit
insn parser only goes up to and including SSSE3.  So I thought that it
didn't need to be patched.

But now I'm not so sure.  Is there any realistic scenario where we try
to run 32 bit code on a CPU that advertises (via CPUID) that it can do
only up until SSSE3, but also contains these NOP instructions?  I think
that's the key question.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #19 from Ivo Raisr  ---
(In reply to Tanya from comment #17)
> (In reply to Julian Seward from comment #9)
> > Committed, r3383, r16415.  Thanks for the patches.
> 
> Julian,
> Would it be possible to patch the 32-bit instruction parser as well?

Sure, no problem. Please provide a patch and test cases.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #18 from Ivo Raisr  ---
(In reply to H.J. Lu from comment #16)
> (In reply to Ivo Raisr from comment #14)
> > And this is a minimalistic reproducer with the latest Valgrind sources:
> > 
> > --
> > #include 
> > 
> > int main ()
> > {
> > __asm__ __volatile__ (".byte 0x65, 0x0f, 0x19, 0xff" :::"cc","memory");
> >   return 0;
> > }
> 
> Does it run on your machine outside of valgrind?

Yes, it does.

The root cause is described in comment #15.
I cannot fix this myself because cet_nops.c uses byte sequences and does not
use assembler mnemonics. My toolchain is not able to properly decode all byte
sequences, even though I (and my CPU) believe they are valid ones.

It would help immensely if you could divide the file (or better split it) in
three sections:
- with gs prefix
- with fs prefix
- with no prefix

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #17 from Tanya  ---
(In reply to Julian Seward from comment #9)
> Committed, r3383, r16415.  Thanks for the patches.

Julian,
Would it be possible to patch the 32-bit instruction parser as well?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[H . J . Lu]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #16 from H.J. Lu  ---
(In reply to Ivo Raisr from comment #14)
> And this is a minimalistic reproducer with the latest Valgrind sources:
> 
> --
> #include 
> 
> int main ()
> {
> __asm__ __volatile__ (".byte 0x65, 0x0f, 0x19, 0xff" :::"cc","memory");
>   return 0;
> }

Does it run on your machine outside of valgrind?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #15 from Ivo Raisr  ---
So the real "culprit" is that on:

amd64/Linux:
guest_amd64_assume_fs_is_const = True
guest_amd64_assume_gs_is_const = True

amd64/Darwin:
guest_amd64_assume_fs_is_const = False
guest_amd64_assume_gs_is_const = True

amd64/Solaris:
guest_amd64_assume_fs_is_const = True
guest_amd64_assume_gs_is_const = True

And amd64 decoder in disInstr_AMD64_WRK(), around lines 32238-32243 fails with:

   /* We have a %fs prefix.  Reject it if there's no evidence in 'vbi'
  that we should accept it. */
   if ((pfx & PFX_FS) && !vbi->guest_amd64_assume_fs_is_const)
  goto decode_failure;

   /* Ditto for %gs prefixes. */
   if ((pfx & PFX_GS) && !vbi->guest_amd64_assume_gs_is_const)
  goto decode_failure;
---


This means that the test cases in cet_nops are probably not valid for all OSes.
Only Linux can have both "fs" and "gs" prefixes, OS X only "gs" and Solaris
only "fs".

I think this could be easily fixed by simple #ifdefery in cet_ops.c.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #14 from Ivo Raisr  ---
And this is a minimalistic reproducer with the latest Valgrind sources:

--
#include 

int main ()
{
__asm__ __volatile__ (".byte 0x65, 0x0f, 0x19, 0xff" :::"cc","memory");
  return 0;
}

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #13 from Ivo Raisr  ---
I've updated GIT repo at sourceware.org.

So the test program under Valgrind crashes at cet_nop.c:42 which is this line:

42  __asm__ __volatile__ (".byte 0xf3, 0x66, 0x64, 0x0f, 0x19, 0xff"
:::"cc","memory");

GDB disassembles it as "gs nop %edi". It is actually the first "nop" with 'gs'
prefix. Perhaps this is the smoking gun?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #12 from Julian Seward  ---
Ivo, I am a bit surprised by this.  The new insn decoder bits are not
dependent on the host's hw caps -- this is handled by dis_ESC_0F, which is
available at any hwcaps level (for example, it handles RDTSC, SYSCALL, and
conditional branches).  Are you sure guest_amd64_toIR.c got recompiled here?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #11 from Tanya  ---
(In reply to Ivo Raisr from comment #10)

Ivo,
I cannot reproduce the issue on the latest Git sources with the 379525 patch
(amd64 linux, gcc 6.2.0). I cannot get the latest SVN sources (firewall blocks
the access, and the "socat" redirection did not work for me.)
Would it be possible to update the Git repository with VALGRIND_3_13_BRANCH? 

Thank you,
Tanya

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #10 from Ivo Raisr  ---
I get the following error output from Valgrind run:

$ ./vg-in-place --tool=none none/tests/amd64/cet_nops
==13161== Nulgrind, the minimal Valgrind tool
==13161== Copyright (C) 2002-2017, and GNU GPL'd, by Nicholas Nethercote.
==13161== Using Valgrind-3.13.0.SVN and LibVEX; rerun with -h for copyright
info
==13161== Command: none/tests/amd64/cet_nops
==13161== 
start doing absolutely nothing ..
vex amd64->IR: unhandled instruction bytes: 0x65 0xF 0x19 0xFF 0x66 0x65 0xF
0x19 0xFF 0xF2
vex amd64->IR:   REX=0 REX.W=0 REX.R=0 REX.X=0 REX.B=0
vex amd64->IR:   VEX=0 VEX.L=0 VEX.n=0x0 ESC=NONE
vex amd64->IR:   PFX.66=0 PFX.F2=0 PFX.F3=0
==13161== valgrind: Unrecognised instruction at address 0x401144.
==13161==at 0x401144: main (cet_nops.c:42)
==13161== Your program just tried to execute an instruction that Valgrind
==13161== did not recognise.  There are two possible reasons for this.
==13161== 1. Your program has a bug and erroneously jumped to a non-code
==13161==location.  If you are running Memcheck and you just saw a
==13161==warning about a bad jump, it's probably your program's fault.
==13161== 2. The instruction is legitimate but Valgrind doesn't handle it,
==13161==i.e. it's Valgrind's fault.  If you think this is the case or
==13161==you are not sure, please let us know and we'll try to fix it.
==13161== Either way, Valgrind will now raise a SIGILL signal which will
==13161== probably kill your program.
==13161== 
==13161== Process terminating with default action of signal 4 (SIGILL): dumping
core
==13161==  Illegal opcode at address 0x401144
==13161==at 0x401144: main (cet_nops.c:42)
==13161== 
./vg-in-place: line 29: 13161: Illegal instruction(coredump)
Illegal Instruction (core dumped)


./vg-in-place --version -v
valgrind-3.13.0.SVN-16420-vex-3384


$ gcc --version
gcc (GCC) 5.4.0
Copyright (C) 2015 Free Software Foundation, Inc.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

Julian Seward  changed:

   What|Removed |Added

 Status|UNCONFIRMED |RESOLVED
 Resolution|--- |FIXED

--- Comment #9 from Julian Seward  ---
Committed, r3383, r16415.  Thanks for the patches.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #8 from Tanya  ---
(In reply to Julian Seward from comment #5)
> (In reply to Tanya from comment #4)
> > Proposed a patch that implements the missing opcodes and allows them to have
> > any prefixes or operands.
> 
> Tanya, thanks for the patch.  Given that we are very close to freezing
> for 3.13 and that this is really our last chance to get decoder fixes in
> for the next several months, I'd like to commit this soon -- this week,
> if the patch is OK.
> 
> Do you have a test case for this?  I think that is important.

Julian,

Added a C application with different opcode/prefix/ModRM combinations (provided
by H.J. Lu). 

I'm not sure how to properly write Valgrind test cases. Attached a test with
all opcode/prefix/ModRM combinations; please let me know if I should only leave
a few.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #7 from Tanya  ---
Created attachment 105677
  --> https://bugs.kde.org/attachment.cgi?id=105677=edit
Files for Valgrind test case

attempt to make Valgrind test case

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tanya]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #6 from Tanya  ---
Created attachment 105676
  --> https://bugs.kde.org/attachment.cgi?id=105676=edit
C test with different opcodes

C application with NOPs with various opcode, prefix and ModRM combinations

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #5 from Julian Seward  ---
(In reply to Tanya from comment #4)
> Proposed a patch that implements the missing opcodes and allows them to have
> any prefixes or operands.

Tanya, thanks for the patch.  Given that we are very close to freezing
for 3.13 and that this is really our last chance to get decoder fixes in
for the next several months, I'd like to commit this soon -- this week,
if the patch is OK.

Do you have a test case for this?  I think that is important.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Tatyana]

https://bugs.kde.org/show_bug.cgi?id=379525

Tatyana  changed:

   What|Removed |Added

 CC||tatyana.a.mine...@intel.com

--- Comment #4 from Tatyana  ---
Created attachment 105595
  --> https://bugs.kde.org/attachment.cgi?id=105595=edit
Proposed patch

Proposed a patch that implements the missing opcodes and allows them to have
any prefixes or operands.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[H . J . Lu]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #3 from H.J. Lu  ---
(In reply to Julian Seward from comment #2)
> HJ, what's the use case here?  Where are these coming from?
> Not from gcc-7.x AFAICT.  So .. where?

NOP opcodes are turned into real instructions on new processors
from time to time. MPX is one example.  CET:

https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf

is a new one.  It is safe for Valgrind to treat those NOPs as NOP.

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Julian Seward]

https://bugs.kde.org/show_bug.cgi?id=379525

--- Comment #2 from Julian Seward  ---
HJ, what's the use case here?  Where are these coming from?
Not from gcc-7.x AFAICT.  So .. where?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Ivo Raisr]

https://bugs.kde.org/show_bug.cgi?id=379525

Ivo Raisr  changed:

   What|Removed |Added

 CC||iv...@ivosh.net

--- Comment #1 from Ivo Raisr  ---
Thank you for the report.
Could you provide a patch which implements the missing functionality?

-- 
You are receiving this mail because:
You are watching all bug changes.

[valgrind] [Bug 379525] Support more x86 nop opcodes

[Pavel Chupin]

https://bugs.kde.org/show_bug.cgi?id=379525

Pavel Chupin  changed:

   What|Removed |Added

 CC||pavel.v.chu...@gmail.com

-- 
You are receiving this mail because:
You are watching all bug changes.