-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
riscv: avoid compressed instructions #2419
base: dev
Are you sure you want to change the base?
Conversation
... and do not throw errors if there are relocs (might be benign)
@@ -267,8 +268,8 @@ def _assembler(): | |||
'ia64': [gas, '-m%ce' % context.endianness[0]], | |||
|
|||
# riscv64-unknown-elf-as supports riscv32 as well as riscv64 | |||
'riscv32': [gas, '-march=rv32gc', '-mabi=ilp32'], | |||
'riscv64': [gas, '-march=rv64gc', '-mabi=lp64'], | |||
'riscv32': [gas, '-march=rv32g', '-mabi=ilp32'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevents you from using compressed instructions when manually writing shellcode and compiling using asm
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, and therefore I am not very sure about this change. I don't know any way to avoid C and yet allow c.* opcodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we thought about adding assembler command line args to context when adding riscv support? Or just asm
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do this like we do with thumb (separate arch) but I doubt whether it is worth it. Maybe we can revisit this one day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to make two different archs? That way you could do asm -c riscv32
and asm -c riscv32c
or something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add this as a context.subarch similar to what is requested for mips and arm? It's not really a sub-architecture though
@@ -60,11 +60,11 @@ Example: | |||
sd t4, -8(sp) | |||
addi sp, sp, -8 | |||
>>> print(enhex(asm(shellcraft.riscv64.pushstr("/bin/sh")))) | |||
b79e39349b8e7e7bb20e938ebe34b60e938efe22233cd1ff6111 | |||
b79e39349b8e7e7b939ece00938ebe34939ede00938efe22233cd1ff130181ff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a new nullbyte here now
I think including the C extension by default is reasonable. All existing riscv64 chips designed to run Linux have the C extension, and main Linux distributions (Debian, Ubuntu, Fedora, Arch Linux...) all use rv64gc as their baseline. When users generate riscv64 shellcode using pwntools, they are most likely running it on a platform that includes the C extension. Including the C extension by default also offers more possibilities for shellcode generation. As @peace-maker pointed out above, a shellcode without the C extension might introduce a null byte. However, riscv32 deserves additional consideration, as it is commonly used in embedded systems where the C extension is not necessarily included. |
... and do not throw errors if there are relocs (might be benign)