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

Bad disassembly of fill-array-data-payload in Dalvik #7376

Closed
cryptax opened this Issue Apr 28, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@cryptax
Copy link

cryptax commented Apr 28, 2017

r2 does not disassemble correctly data array payloads. For example, in the following, the Dalvik bytecode is creating an array data with values { 0xF, 0x74, 0x8 }, but the display below is incorrect:

           0x0007bec8      000301000300.  fill-array-data-payload 1, 3
; .. payload of 4 bytes
            0x0007bed0      0f74           return v116
            0x0007bed2      08000003       move-object/from16 v0, v768
  1. There is not a payload of 4 bytes, but of 3 bytes.
  2. The last two lines should not be interpreted as Dalvik bytecode. They are part of the array data payload. 0f 74 08
  3. The last bytes 0003 are the head for a new fill-array-data-payload. See https://source.android.com/devices/tech/dalvik/dalvik-bytecode#fill-array

Same a little later:

            0x0007bed2      08000003       move-object/from16 v0, v768
            0x0007bed6      0100           move v0, v0
            0x0007bed8      0400           move-wide v0, v0
            0x0007beda      0000           nop
            0x0007bedc      6e72696f0003   invoke-virtual {},  ; 0x6f69

This is a bad disassembly, and should rather show an array data with

              0x6E
              0x72
              0x69
              0x6F

To reproduce, Android/Ztorg sample sha256: 2c546ad7f102f2f345f30f556b8d8162bd365a7f1a52967fce906d46a2b0dac4
However, I assume the bug will show in any Dalvik bytecode using array-data.

@Maijin Maijin added the Android label Apr 28, 2017

@h4ng3r h4ng3r self-assigned this Apr 28, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 6, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 6, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 7, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 7, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 7, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 7, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 7, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 7, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 7, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 7, 2017

@h4ng3r

This comment has been minimized.

Copy link
Contributor

h4ng3r commented May 7, 2017

Hi, @cryptax can you try it again from this PR: #7441?

Remember to use "-e asm.payloads=true" in order to get the payload bytes.

@radare radare closed this in 8bd2882 May 8, 2017

@cryptax

This comment has been minimized.

Copy link
Author

cryptax commented May 9, 2017

@h4ng3r hi sorry for the delay.
This is nearly correct. There is a minor issue: in the following, the payload is 3 bytes, not 4. The last 0x0 should not be part of the payload.

            0x0007bec8      000301000300.  fill-array-data-payload 1, 3
; .. payload of 4 bytes
        0xf
        0x74
        0x8
        0x0      

Here, it is correct, indeed 4 bytes:

            0x0007bed4      000301000400.  fill-array-data-payload 1, 4
; .. payload of 4 bytes
        0x6e
        0x72
        0x69
        0x6f             

@Maijin Maijin reopened this May 9, 2017

@h4ng3r

This comment has been minimized.

Copy link
Contributor

h4ng3r commented May 20, 2017

I'm not sure if it's a problem or not. If the payload size is not 2%==0 it will need 1 more byte in order to be aligned. That's why in the first example from @cryptax the size of the opcode is 3 but payload says 4. For me it make sense... Saying payload of 3 bytes and hiding the last 0x0 is quite lying and it's not r2 style. @radare what do you think?

@radare

This comment has been minimized.

Copy link
Owner

radare commented May 20, 2017

@cryptax

This comment has been minimized.

Copy link
Author

cryptax commented May 22, 2017

@h4ng3r same as @radare. I think you should say:

; .. payload of 3 bytes
       0xf
        0x74
        0x8

and if necessary add something like

     0x0 ; alignment

I agree however this is not a big issue! But saying payload is 4 when it is 3 might lead to misunderstanding for the user.

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 22, 2017

h4ng3r added a commit to h4ng3r/radare2 that referenced this issue May 22, 2017

radare added a commit that referenced this issue May 23, 2017

@radare

This comment has been minimized.

Copy link
Owner

radare commented May 23, 2017

Fixed

@radare radare closed this May 23, 2017

@radare radare added this to the 1.5.0 milestone May 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.