[PowerPC][HTM] Fix disassembling buffer overflow for tabortdc and others

[jsji]

Summary

This was reported in https://bugs.llvm.org/show_bug.cgi?id=41751
llvm-mc aborted when disassembling tabortdc.

This patch try to clean up TM related DAGs.

• Fixes the problem by remove explicit output of cr0, and put it as implicit def.
• Update int_ppc_tbegin pattern to accommodate the implicit def of cr0.
• Update the TCHECK operand and int_ppc_tcheck accordingly.
• Add some builtin test and disassembly tests.
• Remove unused CRRC0/crrc0

[jsji]

@catenacyber Can you please include a testcase for MC please?

[catenacyber]

I guess, can you give me some info/pointer on how to do that ?

[jsji]

You can have a look at llvm/test/MC/Disassembler/PowerPC

[catenacyber]

Adds a test case

[catenacyber]

Is this ok ?
Did I pick the right file ?
How do I run these tests ?

[jsji]

In D61935#1506244, @catenacyber wrote:

Is this ok ?
Did I pick the right file ?
How do I run these tests ?

TBEGIN has Predicates = [HasHTM], and FeatureHTM is Power8SpecificFeatures.
So it would be better to put these in a test file for P8 and above, with RUN line containing -mcpu=pwr8.

Unfortunately, both ppc64-encoding-p8vector.txt and ppc64-encoding-p9vector.txt are not suitable for this due to file naming,
so maybe you should create a file like ppc64-encoding-p8htm.txt.

make check-llvm should run all the tests including this file.
Alternatively, you can use llvm-lit to run it directly for debugging:
llvm-lit llvm/test/MC/Disassembler/PowerPC/ppc64-encoding.txt

[jsji]

lib/Target/PowerPC/PPCInstrHTM.td

30

TBEGIN is NOT the only HTM instructions that has this problem. We should fix all of them together, not just for TBEGIN here. Thanks.

[catenacyber]

lib/Target/PowerPC/PPCInstrHTM.td

30

I agree.
Do you know the complete list ?
I only know the ones that the capstone fuzzer found so far like tabortdc

[jsji]

BTW: You should create the patch with "full diff" when submitting to Phabricator. See https://llvm.org/docs/Phabricator.html

lib/Target/PowerPC/PPCInstrHTM.td

30

You should be able to identify them easily in this td files: any one that try to access variable that is not defined in asm string should be fixed.

[catenacyber]

Fixes all the instructions and bot just tbegin
Move the test(s) to a new file

[jsji]

Thanks, almost good. A few more instructions and test maybe.

lib/Target/PowerPC/PPCInstrHTM.td

79

How about this?

87

How about this?

test/MC/Disassembler/PowerPC/ppc64-encoding-p8htm.txt

6 ↗

(On Diff #201482)

Can we also add test for others?

[catenacyber]

Fixes more instructions
Adds 2 tests

[jsji]

Please make sure you build and test before updating patch. Thanks.

lib/Target/PowerPC/PPCInstrHTM.td

78

You can NOT assign CRRC to Defs.

error: Value 'Defs' of type 'list<Register>' is incompatible with initializer '[CRRC]' of type 'list<RegisterClass>'

Use CR0 too, in UM, treclaim is altering CR0, it was a bug to use CRRC too.

[catenacyber]

Replaces CRRC by CR0
Removes now unused function DecodeCRRC0RegisterClass

[jsji]

Testcase is failing... Have you run test before updating the revision?

It would be great if we can also remove crrc0 in PPCInstrInfo.td and CRRC0 in PPCRegisterInfo.td.

test/MC/Disassembler/PowerPC/ppc64-encoding-p8htm.txt

6 ↗

(On Diff #202668)

without -ppc-asm-full-reg-names, there should not be 'r' before the reg number.

# CHECK: tabortdc. 9, r0, r0
         ^
<stdin>:3:2: note: scanning from here
 tabortdc. 9, 0, 0
 ^

[catenacyber]

Tests are still running on my computer

[jsji]

In D61935#1527546, @catenacyber wrote:

Tests are still running on my computer

Thanks. You can verify your new testcase quickly first with
$ llvm-lit llvm/test/MC/Disassembler/PowerPC/ppc64-encoding-p8htm.txt -v

Then run full make check-all.

[catenacyber]

And there is another failure :
Failing Tests (2):

LLVM :: CodeGen/PowerPC/htm.ll
LLVM :: MC/Disassembler/PowerPC/ppc64-encoding-p8htm.txt

Do you know something about it ?

[catenacyber]

Fixes test adding -ppc-asm-full-reg-names
Removinf CRRC0 from PPCRegisterInfo.td and PPCInstrInfo.td

[catenacyber]

TEST 'LLVM :: CodeGen/PowerPC/htm.ll' is still failing
llc -verify-machineinstrs -mcpu=pwr8 -mattr=+htm < llvm/test/CodeGen/PowerPC/htm.ll | llvm/build/bin/FileCheck llvm/test/CodeGen/PowerPC/htm.ll

[jsji]

In D61935#1527774, @catenacyber wrote:

TEST 'LLVM :: CodeGen/PowerPC/htm.ll' is still failing
llc -verify-machineinstrs -mcpu=pwr8 -mattr=+htm < llvm/test/CodeGen/PowerPC/htm.ll | llvm/build/bin/FileCheck llvm/test/CodeGen/PowerPC/htm.ll

TBEGIN pattern needs update in around line 97.

[catenacyber]

Modifies test test/CodeGen/PowerPC/htm.ll

[catenacyber]

Restores test test/CodeGen/PowerPC/htm.ll
Modifies pattern for tbegin

[jsji]

Doesn't look like a correct pattern update to me.

lib/Target/PowerPC/PPCInstrHTM.td

100

See comments here. How do you express the return value for success/failure now?

106

Are you sure you can just remove the pattern to extract the EQ bits here?

[catenacyber]

Doesn't look like a correct pattern update to me.

This may be beyond my grasp.
I just changed enough to make the tests pass but I do not understand what is going on...
Can you help ?

[jsji]

Sure, I will have a look when I am back from vacation.

[jsji]

I had a quick look, and the problem is that after the fix,
TBEGIN doesn't have any outs now, hence no valid RC, so we can't EXTRACT_SUBREG from this DAG.

We should EXTRACT_SUBREG from CR0 directly, however, we will still need TBEGIN before it to update CR0,
so this is not a single DAG any more.

So it looks like we can NOT use the simple Pat<..> to expand builtin int_ppc_tbegin any more,
we should try to expand it as PPCCustomInserterPseudo.

You should be able to fix it similar to TCHECK_RET.
Let me know if you want me to commandeer this Revision. Thanks.

[catenacyber]

Yes, you can commandeer this revision.
I hope I have been helpful so far.

[jsji]

Commandeer this Revision as discussed.

[jsji]

This patch try to clean up TM related DAGs.

1. Fixes the problem by remove explicit output of cr0, and put it as implicit def.
2. Update int_ppc_tbegin pattern to accommodate the implicit def of cr0.
3. Update the TCHECK operand and int_ppc_tcheck accordingly.
4. Add some builtin test and disassembly tests.
5. Remove unused CRRC0/crrc0

[catenacyber]

This patch looks good to me.

If you want (in another patch) to integrate some fuzzing logic who found this bug, you can take a look at https://github.com/aquynh/capstone/blob/427f695d6bb9cad9d7385a67f8be5d963080806d/suite/fuzz/fuzz_llvm.cpp

[jsji]

Ping...

[jsji]

Ping.. Any other feedback or comments are appreciated. Thanks.

[hfinkel]

LGTM

llvm/lib/Target/PowerPC/PPCInstrHTM.td

84 ↗

(On Diff #203824)

Comment to be removed?

[jsji]

Thanks, will remove that comment in commit.

lib/Target/PowerPC/PPCInstrHTM.td

 
 let Predicates = [HasHTM] in {
 
+let Defs = [CR0] in
 def TBEGIN : XForm_htm0 <31, 654,
-                         (outs crrc0:$ret), (ins u1imm:$R), "tbegin. $R", IIC_SprMTSPR, []>;
+                         (outs), (ins u1imm:$R), "tbegin. $R", IIC_SprMTSPR, []>;
 
 def TEND : XForm_htm1 <31, 686,
                        (outs crrc0:$ret), (ins u1imm:$A), "tend. $A", IIC_SprMTSPR, []>;

test/MC/Disassembler/PowerPC/ppc64-encoding.txt

 
 # CHECK: stop
 0x4c 0x00 0x02 0xe4
+
+# CHECK: tbegin. 1
+0x7c 0x20 0x05 0x1d