Skip to content

Conversation

@catenacyber
Copy link
Contributor

@aquynh
Copy link
Collaborator

aquynh commented May 3, 2019

Interesting, how did you come up with this fix?

@catenacyber
Copy link
Contributor Author

From https://github.com/llvm-mirror/llvm/blob/2ce4ce4f9254350e32ad822d9394d3a1c82b0e12/lib/Target/PowerPC/PPCInstrHTM.td#L58

It looks like the three operands are u5imm, gprc, u5imm
It looked like that in PPCGenAsmWriter.inc as well
Hence the change in PPCGenDisassemblerTables.inc, and the confirmation from onlinedisassembler.com

Does it look good ?

@aquynh
Copy link
Collaborator

aquynh commented May 3, 2019

we are basing on llvm 7.0.1. if you build llvm, you will see that the related code in like below.

  case 73:
    tmp = fieldFromInstruction(insn, 21, 5);
    if (DecodeCRRC0RegisterClass(MI, tmp, Address, Decoder) == MCDisassembler::Fail) { return MCDisassembler::Fail; }
    tmp = fieldFromInstruction(insn, 16, 5);
    if (DecodeGPRCRegisterClass(MI, tmp, Address, Decoder) == MCDisassembler::Fail) { return MCDisassembler::Fail; }
    tmp = fieldFromInstruction(insn, 11, 5);
    if (DecodeGPRCRegisterClass(MI, tmp, Address, Decoder) == MCDisassembler::Fail) { return MCDisassembler::Fail; }
    return S;
  case 74:
    tmp = fieldFromInstruction(insn, 21, 5);
    if (DecodeCRRC0RegisterClass(MI, tmp, Address, Decoder) == MCDisassembler::Fail) { return MCDisassembler::Fail; }
    tmp = fieldFromInstruction(insn, 16, 5);
    if (DecodeGPRCRegisterClass(MI, tmp, Address, Decoder) == MCDisassembler::Fail) { return MCDisassembler::Fail; }
    tmp = fieldFromInstruction(insn, 11, 5);
    if (decodeUImmOperand<5>(MI, tmp, Address, Decoder) == MCDisassembler::Fail) { return MCDisassembler::Fail; }
    return S;

so we just "port" this code to Capstone, as is.

can you provide the related input (for cstool) of this case?

@catenacyber
Copy link
Contributor Author

cstool -d ppc64be "7d 20 06 5d"
cstool -d ppc64beqpx "7c 20 1e dd"

With this LLVM code, there is at least the problem that you can get an overflow reading CRRC0 register number on 5 bits, when there are only 8 of them (so overflow, when you use the number/index to get the register name/info)

@aquynh
Copy link
Collaborator

aquynh commented May 3, 2019

this input "7d 20 06 5d" is tabortdc. 9, r0, r0 on onlinedisassembler, but crashes both LLVM 7.0.1 & 8.0

@catenacyber
Copy link
Contributor Author

Did you report it to them ?

@aquynh
Copy link
Collaborator

aquynh commented May 4, 2019

Not yet, do you want to report yourself?

@aquynh
Copy link
Collaborator

aquynh commented May 4, 2019

Can you create a separate PR for the fuzzer, so i can merge it independently?

@catenacyber
Copy link
Contributor Author

@kcc aren't you fuzzing LLVM ?
https://bugs.llvm.org/show_bug.cgi?id=41751

@kcc
Copy link

kcc commented May 6, 2019

We do fuzz various bits of LLVM on oss-fuzz, but I don't know whether we fuzz this particular code path.
Looks like we don't, actually.
https://github.com/google/oss-fuzz/blob/master/projects/llvm/build.sh

Help is more than welcome!

@catenacyber
Copy link
Contributor Author

@kcc you can take a look here :
https://github.com/aquynh/capstone/blob/427f695d6bb9cad9d7385a67f8be5d963080806d/suite/fuzz/fuzz_llvm.cpp

There, I tried to do differential fuzzing between capstone and llvm.
Turned out not so relevant as output is not normalized.
But this was still fuzzing llvm LLVMDisasmInstruction
So, how can I help ?

@catenacyber
Copy link
Contributor Author

Can you create a separate PR for the fuzzer, so i can merge it independently?
@aquynh
I just did #1473
And reverted the commit in this branch (I can remove it and force-push if you prefer)

@aquynh
Copy link
Collaborator

aquynh commented May 7, 2019

@catenacyber, this is fine, because eventually i will just "squash & merge", and all the reverted commits will be gone.

@catenacyber
Copy link
Contributor Author

Followed by #1510

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants