-
Notifications
You must be signed in to change notification settings - Fork 1.4k
executor, sys/linux, pkg: enable syzos for riscv64 #6656
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
base: master
Are you sure you want to change the base?
Conversation
1052267 to
4fe8ded
Compare
executor/common_kvm_riscv64_syzos.h
Outdated
| return; | ||
| if (cmd->size > size) | ||
| return; | ||
| switch (cmd->call) { |
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.
Have you tried building with Clang? It is actually pretty aggressively turning switches into jump tables.
Perhaps it will start biting soon.
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.
Ok, I mainly based the adaptation on arm64. I’ll modify it to use if-else statements instead.
| // The most common reason for using data relocaions is accessing global variables and constants. | ||
| // Sometimes the compiler may choose to emit a read-only constant to zero-initialize a structure | ||
| // or to generate a jump table for a switch statement. | ||
| static void validate_guest_code(void* mem, size_t size) |
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.
Out of curiosity, how did you test this code?
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 also suggest extending tools/check-syzos.sh.
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.
Since ADRP in arm64 is similar to AUIPC in riscv64, I modified it to AUIPC for riscv64. I'll add a test for this in tools/check-syzos.sh going forward.
executor/common_kvm_riscv64.h
Outdated
| #define AUIPC_OPCODE 0x17 | ||
| #define AUIPC_OPCODE_MASK 0x7f | ||
|
|
||
| // Code loading SyzOS into guest memory does not handle data relocations (see |
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.
Please change to "SYZOS".
The naming is still inconsistent across the codebase, but I've decided "SYZOS" is better than "SyzOS", and now the former prevails.
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.
Acknowledged.
executor/common_kvm_riscv64.h
Outdated
| // Setup executor guest code. | ||
| struct addr_size host_text = alloc_guest_mem(&allocator, 4 * KVM_PAGE_SIZE); | ||
| install_syzos_code(host_text.addr, host_text.size); | ||
| vm_set_user_memory_region(vmfd, slot++, KVM_MEM_READONLY, SYZOS_ADDR_EXECUTOR_CODE, host_text.size, (uintptr_t)host_text.addr); |
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.
Please take a look at how setup_vm() is implemented for amd64.
While this code (apparently based on the executor/common_kvm_arm64.h) is fine, the x86 implementation does a better job factoring out the memory layout configuration.
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.
Yes, you're right. I have modified the riscv64 implementation to refer to the x86 implementation.
| @@ -0,0 +1,19 @@ | |||
| # | |||
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.
Have you tried generating C reproducers out of these programs and ensuring that the assertions pass?
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.
Yes, I have verified this. I first generated a C reproducer using
./bin/syz-prog2c -os=linux -arch=riscv64 -repeat=1 -prog=sys/linux/test/riscv64-syz_kvm_setup_syzos_vm-csrw > repro.c.
The generated repro had a small type typo (uint64_t_t), which I manually fixed to uint64_t to make it compile.
I then cross-compiled the program for riscv64, ran the resulting binary inside a QEMU riscv64 VM, and the syzos assertions passed as expected.
| // We use prime numbers multiplied by 10 to prevent this behavior. | ||
| // Remember these constants must match those in sys/linux/dev_kvm_riscv64.txt. | ||
| typedef enum { | ||
| SYZOS_API_UEXIT = 0, |
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.
As noted elsewhere, executor/common_kvm_amd64.h is more modern and it deprecates some decisions originally suggested in executor/common_kvm_arm64.h
In particular, instead of trying to trick the compiler with prime numbers, we rely on if-chains and volatiles.
It is still good to group related API commands together and leave some space between them though.
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.
Acknowledged!
executor/common_kvm_riscv64_syzos.h
Outdated
| uint64 size; | ||
| }; | ||
|
|
||
| struct api_call_uexit { |
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 suggest using api_call_1 instead.
It was silly of me to try and invent a separate struct for each API call.
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.
OK, I'll change it.
executor/common_kvm_riscv64_syzos.h
Outdated
| uint32 insns[]; | ||
| }; | ||
|
|
||
| struct api_call_1 { |
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.
While at it, can you please move api_call_header, api_call_1, api_call_2 etc. to executor/common_kvm_syzos.h? I think it's long overdue.
You can probably keep api_call_code here for now, as it is arch-specific.
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.
OK, I'll do it.
executor/common_kvm_riscv64_syzos.h
Outdated
|
|
||
| struct api_call_1 { | ||
| struct api_call_header header; | ||
| uint64 arg; |
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'll need to change this to args[1] at some point, but don't bother now.
| GUEST_CODE static noinline void guest_execute_code(uint32* insns, uint64 size) | ||
| { | ||
| asm volatile("fence.i" :: | ||
| : "memory"); |
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.
Out of curiosity, did clang-format wrap this line?
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 ran make format and make generate before committing.
| if (size > guest_mem_size) | ||
| size = guest_mem_size; | ||
| memcpy(host_mem, text, size); | ||
| memcpy(host_mem + page_size, (void*)guest_unexp_trap, KVM_PAGE_SIZE); |
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.
guest_unexp_trap() is a guest function, how about moving it into the SYZOS header?
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 it’s better to keep guest_unexp_trap() here.
SYZOS guest functions are meant to be mutable via syz_kvm_add_vcpu parameters, while guest_unexp_trap() is part of the fixed vCPU setup path and not intended to be mutated.
What do you think ?~
99a3c84 to
f1828c2
Compare
|
Thanks for the review and suggestions. I’ve made the changes and updated the commit. Please take a look. |
7317dc0 to
42cebab
Compare
This patch enables syzos for riscv64 and implements the corresponding pseudo syscalls. Pseudo syscalls: - syz_kvm_setup_syzos_vm - syz_kvm_add_vcpu - syz_kvm_assert_syzos_uexit Syzos guest support: - guest_uexit - guest_execute_code - guest_handle_csrr and guest_handle_csrw Test seeds: - riscv64-syz_kvm_setup_syzos_vm - riscv64-syz_kvm_setup_syzos_vm-csrr - riscv64-syz_kvm_setup_syzos_vm-csrw
42cebab to
0dd74fd
Compare
This patch enables syzos for riscv64 and implements the corresponding pseudo syscalls.
Pseudo syscalls:
Syzos guest support:
Test seeds:
The partial printout information of
syz-execprog -debug -threaded=0 riscv64-syz_kvm_setup_syzos_vm/riscv64-syz_kvm_setup_syzos_vm-csrr/riscv64-syz_kvm_setup_syzos_vm-csrwis as follows. It can be seen that the executions were successful.Before sending a pull request, please review Contribution Guidelines:
https://github.com/google/syzkaller/blob/master/docs/contributing.md