This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jsji on May 15 2019, 2:37 AM.

Details

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

Diff Detail

Event Timeline

catenacyber created this revision.May 15 2019, 2:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 2:37 AM
jsji added a comment.May 15 2019, 2:24 PM

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

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

jsji added a comment.May 16 2019, 12:07 PM

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

Adds a test case

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

jsji added a comment.May 20 2019, 11:35 AM

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 added inline comments.May 20 2019, 11:55 AM
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 added inline comments.May 22 2019, 2:48 AM
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 added a comment.May 23 2019, 9:28 AM

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.

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

jsji added a comment.May 28 2019, 7:38 PM

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?

Fixes more instructions
Adds 2 tests

jsji requested changes to this revision.May 30 2019, 2:18 PM

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.

This revision now requires changes to proceed.May 30 2019, 2:18 PM

Replaces CRRC by CR0
Removes now unused function DecodeCRRC0RegisterClass

jsji requested changes to this revision.Jun 3 2019, 8:13 AM

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
 ^
This revision now requires changes to proceed.Jun 3 2019, 8:13 AM

Tests are still running on my computer

jsji added a comment.Jun 3 2019, 8:43 AM

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.

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 ?

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

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 added a comment.Jun 3 2019, 11:07 AM

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.

Modifies test test/CodeGen/PowerPC/htm.ll

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

jsji added a comment.Jun 4 2019, 7:05 AM

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?

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 added a comment.Jun 4 2019, 9:14 PM

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

jsji added a comment.Jun 7 2019, 10:19 AM

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.

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

jsji commandeered this revision.Jun 10 2019, 7:40 AM
jsji edited reviewers, added: catenacyber; removed: jsji.

Commandeer this Revision as discussed.

jsji updated this revision to Diff 203824.EditedJun 10 2019, 7:42 AM

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
jsji retitled this revision from Fixes PPC64 Tbegin disassembling to [PowerPC][HTM] Fix disassembling buffer overflow for tabortdc and others.Jun 10 2019, 7:44 AM
jsji added a reviewer: kbarton.
catenacyber added a comment.EditedJun 11 2019, 3:32 AM

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 edited the summary of this revision. (Show Details)Jun 17 2019, 7:57 PM
jsji added a reviewer: lei.Jun 21 2019, 6:49 AM
jsji added a comment.Jun 24 2019, 7:03 AM

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

hfinkel accepted this revision.Jun 25 2019, 7:00 PM

LGTM

llvm/lib/Target/PowerPC/PPCInstrHTM.td
84 ↗(On Diff #203824)

Comment to be removed?

This revision is now accepted and ready to land.Jun 25 2019, 7:00 PM
jsji marked an inline comment as done.Jun 26 2019, 1:42 PM

Thanks, will remove that comment in commit.

This revision was automatically updated to reflect the committed changes.