[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

80

How about this?

89

How about this?

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

7

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

7

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/Disassembler/PPCDisassembler.cpp

 }
 
 static DecodeStatus DecodeCRRCRegisterClass(MCInst &Inst, uint64_t RegNo,
                                             uint64_t Address,
                                             const void *Decoder) {
   return decodeRegisterClass(Inst, RegNo, CRRegs);
 }
 
-static DecodeStatus DecodeCRRC0RegisterClass(MCInst &Inst, uint64_t RegNo,
-                                            uint64_t Address,
-                                            const void *Decoder) {
-  return decodeRegisterClass(Inst, RegNo, CRRegs);
-}
-
 static DecodeStatus DecodeCRBITRCRegisterClass(MCInst &Inst, uint64_t RegNo,
                                             uint64_t Address,
                                             const void *Decoder) {
   return decodeRegisterClass(Inst, RegNo, CRBITRegs);
 }
 
 static DecodeStatus DecodeF4RCRegisterClass(MCInst &Inst, uint64_t RegNo,
                                             uint64_t Address,

lib/Target/PowerPC/PPCInstrHTM.td

 
 let hasSideEffects = 1 in {
 def TCHECK_RET : PPCCustomInserterPseudo<(outs crrc:$out), (ins), "#TCHECK_RET", []>;
 }
 
 
 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, []>;
 
+let Defs = [CR0] in
 def TEND : XForm_htm1 <31, 686,
-                       (outs crrc0:$ret), (ins u1imm:$A), "tend. $A", IIC_SprMTSPR, []>;
+                       (outs), (ins u1imm:$A), "tend. $A", IIC_SprMTSPR, []>;
 
+let Defs = [CR0] in
 def TABORT : XForm_base_r3xo <31, 910,
-                              (outs crrc0:$ret), (ins gprc:$A), "tabort. $A", IIC_SprMTSPR,
+                              (outs), (ins gprc:$A), "tabort. $A", IIC_SprMTSPR,
                               []>, isDOT {
   let RST = 0;
   let B = 0;
 }
 
+let Defs = [CR0] in
 def TABORTWC : XForm_base_r3xo <31, 782,
-                                (outs crrc0:$ret), (ins u5imm:$RTS, gprc:$A, gprc:$B),
+                                (outs), (ins u5imm:$RTS, gprc:$A, gprc:$B),
                                 "tabortwc. $RTS, $A, $B", IIC_SprMTSPR, []>,
                                 isDOT;
 
+let Defs = [CR0] in
 def TABORTWCI : XForm_base_r3xo <31, 846,
-                                 (outs crrc0:$ret), (ins u5imm:$RTS, gprc:$A, u5imm:$B),
+                                 (outs), (ins u5imm:$RTS, gprc:$A, u5imm:$B),
                                  "tabortwci. $RTS, $A, $B", IIC_SprMTSPR, []>,
                                  isDOT;
 
+let Defs = [CR0] in
 def TABORTDC : XForm_base_r3xo <31, 814,
-                                (outs crrc0:$ret), (ins u5imm:$RTS, gprc:$A, gprc:$B),
+                                (outs), (ins u5imm:$RTS, gprc:$A, gprc:$B),
                                 "tabortdc. $RTS, $A, $B", IIC_SprMTSPR, []>,
                                 isDOT;
 
+let Defs = [CR0] in
 def TABORTDCI : XForm_base_r3xo <31, 878,
-                                 (outs crrc0:$ret), (ins u5imm:$RTS, gprc:$A, u5imm:$B),
+                                 (outs), (ins u5imm:$RTS, gprc:$A, u5imm:$B),
                                  "tabortdci. $RTS, $A, $B", IIC_SprMTSPR, []>,
                                  isDOT;
 
+let Defs = [CR0] in
 def TSR : XForm_htm2 <31, 750,
-                      (outs crrc0:$ret), (ins u1imm:$L), "tsr. $L", IIC_SprMTSPR, []>,
+                      (outs), (ins u1imm:$L), "tsr. $L", IIC_SprMTSPR, []>,
                       isDOT;
 
 def TCHECK : XForm_htm3 <31, 718,
                         (outs), (ins crrc:$BF), "tcheck $BF", IIC_SprMTSPR, []>;
 
 
+let Defs = [CR0] in
 def TRECLAIM : XForm_base_r3xo <31, 942,
-                                (outs crrc:$ret), (ins gprc:$A), "treclaim. $A",
+                                (outs), (ins gprc:$A), "treclaim. $A",
                                 IIC_SprMTSPR, []>,
                                 isDOT {
   let RST = 0;
   let B = 0;
 }
 
+let Defs = [CR0] in
 def TRECHKPT : XForm_base_r3xo <31, 1006,
-                                (outs crrc:$ret), (ins), "trechkpt.", IIC_SprMTSPR, []>,
+                                (outs), (ins), "trechkpt.", IIC_SprMTSPR, []>,
                                 isDOT {
   let RST = 0;
   let A = 0;
   let B = 0;
 }
 
 // Builtins
 
 // All HTM instructions, with the exception of tcheck, set CR0 with the
 // value of the MSR Transaction State (TS) bits that exist before the
 // instruction is executed.  For tbegin., the EQ bit in CR0 can be used
 // to determine whether the transaction was successfully started (0) or
 // failed (1).  We use an XORI pattern to 'flip' the bit to match the
 // tbegin builtin API which defines a return value of 1 as success.
 
 def : Pat<(int_ppc_tbegin i32:$R),
-           (XORI
-             (EXTRACT_SUBREG (
-               TBEGIN (HTM_get_imm imm:$R)), sub_eq),
-            1)>;
+           (TBEGIN (HTM_get_imm imm:$R))>;
 
 def : Pat<(int_ppc_tend i32:$R),
           (TEND (HTM_get_imm imm:$R))>;
 
 
 def : Pat<(int_ppc_tabort i32:$R),
           (TABORT $R)>;
 

lib/Target/PowerPC/PPCInstrInfo.td

def crbitrc : RegisterOperand<CRBITRC> {
   let ParserMatchClass = PPCRegCRBITRCAsmOperand;
 }
 def PPCRegCRRCAsmOperand : AsmOperandClass {
   let Name = "RegCRRC"; let PredicateMethod = "isCCRegNumber";
 }
 def crrc : RegisterOperand<CRRC> {
   let ParserMatchClass = PPCRegCRRCAsmOperand;
 }
-def crrc0 : RegisterOperand<CRRC0> {
-  let ParserMatchClass = PPCRegCRRCAsmOperand;
-}
 
 def PPCRegSPERCAsmOperand : AsmOperandClass {
   let Name = "RegSPERC"; let PredicateMethod = "isRegNumber";
 }
 def sperc : RegisterOperand<SPERC> {
   let ParserMatchClass = PPCRegSPERCAsmOperand;
 }
 def PPCRegSPE4RCAsmOperand : AsmOperandClass {

lib/Target/PowerPC/PPCRegisterInfo.td

(add CR2LT, CR2GT, CR2EQ, CR2UN,
        CR1LT, CR1GT, CR1EQ, CR1UN,
        CR0LT, CR0GT, CR0EQ, CR0UN)> {
   let Size = 32;
 }
 
 def CRRC : RegisterClass<"PPC", [i32], 32, (add CR0, CR1, CR5, CR6,
                                                 CR7, CR2, CR3, CR4)>;
 
-def CRRC0 : RegisterClass<"PPC", [i32], 32, (add CR0)>;
-
 // The CTR registers are not allocatable because they're used by the
 // decrement-and-branch instructions, and thus need to stay live across
 // multiple basic blocks.
 def CTRRC : RegisterClass<"PPC", [i32], 32, (add CTR)> {
   let isAllocatable = 0;
 }
 def CTRRC8 : RegisterClass<"PPC", [i64], 64, (add CTR8)> {
   let isAllocatable = 0;
 }
 
 def VRSAVERC : RegisterClass<"PPC", [i32], 32, (add VRSAVE)>;
 def CARRYRC : RegisterClass<"PPC", [i32], 32, (add CARRY, XER)> {
   let CopyCost = -1;
 }
 

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

+# RUN: llvm-mc --disassemble %s -triple powerpc64-unknown-unknown -mcpu=pwr8 -ppc-asm-full-reg-names | FileCheck %s
+
+# CHECK: tbegin. 1
+0x7c 0x20 0x05 0x1d
+
+# CHECK: tabortdc. 9, r0, r0
+0x7d 0x20 0x06 0x5d
+
+# CHECK: tabortdci. 1, r0, 3
+0x7c 0x20 0x1e 0xdd
+
+# CHECK: tsr. 1
+0x7c 0x20 0x05 0xdd