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

Fix asm() instruction consistency #2144

Merged
merged 4 commits into from Dec 28, 2022

Conversation

rtlcopymemory
Copy link
Contributor

Fix asm() instruction consistency

Expected Behaviour

When using either asm() with the vma parameter or the ELF.asm() function with an instruction like nop I expect it to write 1 byte 0x90 at the specified position.

Actual behaviour

Depending on the memory address specified (either as vma or as first parameter of ELF.asm()) it will add padding nop bytes before the instruction. More specifically, if the address is not a multiple of 4 (address % 4 != 0) it will add 4 - (address % 4) padding bytes making it sometimes hard to patch instructions smaller than 4 bytes.

Example Script

Running this script will quickly demonstrate the effect

from pwn import *

for i in range(17):
    print(f"{i}", asm("INT 3", vma=i))

Output before changes

0 b'\xcc'
1 b'f\x90\x90\xcc'
2 b'f\x90\xcc'
3 b'\x90\xcc'
4 b'\xcc'
5 b'f\x90\x90\xcc'
6 b'f\x90\xcc'
7 b'\x90\xcc'
8 b'\xcc'
9 b'f\x90\x90\xcc'
10 b'f\x90\xcc'
11 b'\x90\xcc'
12 b'\xcc'
13 b'f\x90\x90\xcc'
14 b'f\x90\xcc'
15 b'\x90\xcc'
16 b'\xcc'

Output after changes

0 b'\xcc'
1 b'\xcc'
2 b'\xcc'
3 b'\xcc'
4 b'\xcc'
5 b'\xcc'
6 b'\xcc'
7 b'\xcc'
8 b'\xcc'
9 b'\xcc'
10 b'\xcc'
11 b'\xcc'
12 b'\xcc'
13 b'\xcc'
14 b'\xcc'
15 b'\xcc'
16 b'\xcc'

Testing

I was not able to start the whole test suite but I tried testing at least the asm part which doesn't start on only one architecture (it isn't failing, I'm missing some dependency).
The Docker image also seems to fail to build at the apt update step.
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?

This, therefore, might require at least someone who can run all the tests to test them again

(First time contributing to a project, I hope I did this right, I apologize if I made a mistake either choosing the wrong target branch or in the rest of the pull request)

pwnlib/asm.py Outdated Show resolved Hide resolved
Co-authored-by: Arusekk <arek_koz@o2.pl>
@Arusekk Arusekk merged commit b771fc5 into Gallopsled:dev Dec 28, 2022
@Arusekk
Copy link
Member

Arusekk commented Dec 28, 2022

TIL: apparently thumb also needs .p2align 2, the CI is failing now. :-)

Arusekk added a commit that referenced this pull request Dec 28, 2022
This is why CI should always be green.
Make it green, dear contributor, because the maintainers might merge
something without checking if it is green.  Please.
Arusekk added a commit that referenced this pull request Dec 28, 2022
This is why CI should always be green.
Make it green, dear contributor, because the maintainers might merge
something without checking if it is green.  Please.
gogo2464 pushed a commit to gogo2464/pwntools that referenced this pull request Sep 10, 2023
* sets p2align header to 0 for x86-32 x86-64 and thumb

* updates changelog

* adds PR link to changelog

* Update pwnlib/asm.py

Co-authored-by: Arusekk <arek_koz@o2.pl>

Co-authored-by: Arusekk <arek_koz@o2.pl>
gogo2464 pushed a commit to gogo2464/pwntools that referenced this pull request Sep 10, 2023
This is why CI should always be green.
Make it green, dear contributor, because the maintainers might merge
something without checking if it is green.  Please.
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.

None yet

2 participants