Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trouble with ARM Operand-Shift #246

Closed
jabba2989 opened this issue Jan 9, 2015 · 6 comments
Closed

Trouble with ARM Operand-Shift #246

jabba2989 opened this issue Jan 9, 2015 · 6 comments

Comments

@jabba2989
Copy link

Hello,

i'm having trouble with following instruction:

Platform: XMC4500 (ARM Cortex M4F)
Capstone Vers. 3.0 with Python Binding
ldr.w   pc, [r2, r3, lsl #2] (f852 f023 Thumb2)

The following code (taken from the python test examples),

    THUMB_CODE2 = 
    "\x02\xea\x83\x02\x52\xf8\x23\xf0\xbd\xe8\x00\x88\xd1\xe8\x00\xf0\x18\xbf\xad\xbf\xf3\xff\x0b\x0c
    \x86\xf3\x00\x89\x80\xf3\x00\x8c\x4f\xfa\x99\xf6\xd0\xff\xa2\x01"

    md = Cs(CS_ARCH_ARM, CS_MODE_THUMB+CS_MODE_MCLASS)
    md.detail = True
    for insn in md.disasm(THUMB_CODE2, 0x1000):
        print("0x%X: %s %s" % (insn.address,insn.mnemonic,insn.op_str))  
        if len(insn.operands) > 0:
            print("\top_count: %u" % len(insn.operands))
            c = 0
            for i in insn.operands:
                if i.type == ARM_OP_REG:
                    print("\t\toperands[%u].type: REG = %s" % (c, insn.reg_name(i.reg)))
                if i.type == ARM_OP_IMM:
                    print("\t\toperands[%u].type: IMM = 0x%s" % (c, to_x_32(i.imm)))
                if i.type == ARM_OP_PIMM:
                    print("\t\toperands[%u].type: P-IMM = %u" % (c, i.imm))
                if i.type == ARM_OP_CIMM:
                    print("\t\toperands[%u].type: C-IMM = %u" % (c, i.imm))
                if i.type == ARM_OP_FP:
                    print("\t\toperands[%u].type: FP = %f" % (c, i.fp))
                if i.type == ARM_OP_SYSREG:
                    print("\t\toperands[%u].type: SYSREG = %u" % (c, i.reg))
                if i.type == ARM_OP_SETEND:
                    if i.setend == ARM_SETEND_BE:
                        print("\t\toperands[%u].type: SETEND = be" % c)
                    else:
                        print("\t\toperands[%u].type: SETEND = le" % c)
                if i.type == ARM_OP_MEM:
                    print("\t\toperands[%u].type: MEM" % c)
                    if i.mem.base != 0:
                        print("\t\t\toperands[%u].mem.base: REG = %s" \
                            % (c, insn.reg_name(i.mem.base)))
                    if i.mem.index != 0:
                        print("\t\t\toperands[%u].mem.index: REG = %s" \
                            % (c, insn.reg_name(i.mem.index)))
                        print i.shift.type
                    if i.mem.scale != 1:
                        print("\t\t\toperands[%u].mem.scale: %u" \
                            % (c, i.mem.scale))
                    if i.mem.disp != 0:
                        print("\t\t\toperands[%u].mem.disp: 0x%s" \
                            % (c, to_x_32(i.mem.disp)))

                if i.shift.type != ARM_SFT_INVALID and i.shift.value:
                    print("\t\t\tShift: %u = %u" \
                        % (i.shift.type, i.shift.value))
                if i.vector_index != -1:
                    print("\t\t\toperands[%u].vector_index = %u" %(c, i.vector_index))
                if i.subtracted:
                    print("\t\t\toperands[%u].subtracted = True")

                c += 1

prints (only a part of):

0x1004: ldr.w pc, [r2, r3, lsl #2]
    op_count: 2
        operands[0].type: REG = pc
            Shift: 2 = 2
        operands[1].type: MEM
            operands[1].mem.base: REG = r2
            operands[1].mem.index: REG = r3

As you can see the logical shift which is performed on R3, is instead associated with operand[0] which is the PC. Is this an intended behavior? I'm aware the the second operand is a memory access and there is no attribute regarding an shift operation but i somehow need to make a proper connection between the shift operation and the index register. I'm not exactly sure if this is a bug or if i'm missing something.
Thank's in advance

@aquynh
Copy link
Collaborator

aquynh commented Jan 9, 2015

yes this is a bug. i think the shift should be done on R3, not on PC.

how do you think we should fix this issue without having to change the structure of cs_arm_op ?

@jabba2989
Copy link
Author

Unfortunatly i'm not familiar with the source. From what I can see is, that an operand shift never effects the whole memory operand and it is never applied to the base register. This implies that only the index register can be optionally shifted and so I guess for the moment it should be enough to fix the wrong association from the PC to the mem operand.

@aquynh
Copy link
Collaborator

aquynh commented Jan 12, 2015

working on a fix now. it seems the only type of shift inside a memory address is left-shift on the last (index) register. do you see any exception to this?

thanks.

@aquynh
Copy link
Collaborator

aquynh commented Jan 12, 2015

it seems the only type of shift in memory operand is left-shift.

this issue has been fixed in the next branch, with your name credited: 706b808. please confirm the bug is solved.

note: the next branch has API version 4.0, which is incompatible with 3.x version, so besides the core, you need to reinstall the Python binding, too.

@aquynh
Copy link
Collaborator

aquynh commented Jan 12, 2015

to be sure, reinstall the core & Python binding like followings if you are on *nix (Linux, OSX, BSD, etc):

$ ./make.sh
$ sudo ./make.sh install
$ cd bindings/python
$ sudo make install

@jabba2989
Copy link
Author

Yes it seems LSL is the only shift which is applied to the index register (for thumb memory instructions only!). Tested and it works. Thanks for your effort!

@aquynh aquynh closed this as completed Jan 13, 2015
disconnect3d added a commit to disconnect3d/capstone that referenced this issue Jan 1, 2019
For a more detailed description, see issue capstone-engine#1317.

Release 4.0.0 introduced a new field for ARM operands:
`operand.mem.lshift`. This field was supposed to be a bug fix for capstone-engine#246.
The capstone-engine#246 issue has been fixed in the meantime and the proper shift value
was stored in `operand.shift.value`.

The 4.0.0 changes created a regression in which `operand.shift.value`
was not set for a `tbh [r0, r1, lsl capstone-engine#1]` instruction on ARM and the
value was set in a `operand.mem.lshift` field instead.

As the regression broke some of users codebase (e.g. in
[manticore](trailofbits/manticore#1312) project), we fix it by setting
`operand.shift.value` back again.

As a result, the shift value is set in two fields: `operand.shift.value`
and `operand.mem.lshift`. As the `operand.shift` also stores a `.type`
field, we might want to deprecate `operand.mem.lshift` in the future.
aquynh pushed a commit that referenced this issue Jan 2, 2019
* Fixes #1317 - arm thb operand.shift.value

For a more detailed description, see issue #1317.

Release 4.0.0 introduced a new field for ARM operands:
`operand.mem.lshift`. This field was supposed to be a bug fix for #246.
The #246 issue has been fixed in the meantime and the proper shift value
was stored in `operand.shift.value`.

The 4.0.0 changes created a regression in which `operand.shift.value`
was not set for a `tbh [r0, r1, lsl #1]` instruction on ARM and the
value was set in a `operand.mem.lshift` field instead.

As the regression broke some of users codebase (e.g. in
[manticore](trailofbits/manticore#1312) project), we fix it by setting
`operand.shift.value` back again.

As a result, the shift value is set in two fields: `operand.shift.value`
and `operand.mem.lshift`. As the `operand.shift` also stores a `.type`
field, we might want to deprecate `operand.mem.lshift` in the future.

* Add changelog stub
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

No branches or pull requests

2 participants