-
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.
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.
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.
That's because executor code is using uint32, uint64 etc. instead of uint32_t, uint64_t. Just make sure you don't have uint64_t in the SYZOS headers.
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.
| 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 ?~
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.
SYZOS guest functions are meant to be mutable via syz_kvm_add_vcpu parameters
I don't understand. All of SYZOS code is immutable, we can only pass parameters to it.
What is the purpose of guest_unexp_trap(), is it something like a default interrupt handler?
In that case, it is probably meant to live in SYZOS.
Also, right now these functions are not marked as GUEST_CODE. If e.g. guest_unexp_trap() happens to call sbi_ecall() instead of inlining it, everything will break, because sbi_ecall() won't be in the guest address space.
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
| size_t size = (char*)&__stop_guest - (char*)&__start_guest; | ||
| if (size > mem_size) | ||
| fail("SyzOS size exceeds guest memory"); | ||
| fail("SYZOS size exceeds guest 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.
Thanks for fixing the naming in the existing code, but please put it into a separate commit.
| SYZOS_API_STOP, // Must be the last one | ||
| } syzos_api_id; | ||
|
|
||
| struct api_call_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.
Please make this a separate commit that only touches x86/ARM and brings no functional change.
|
|
||
| // Code loading SyzOS into guest memory does not handle data relocations (see | ||
| // https://github.com/google/syzkaller/issues/5565), so SyzOS will crash soon after encountering an | ||
| // 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.
Same here - please group all s/SyzOS/SYZOS into a separate commit.
| return -1; | ||
| } | ||
|
|
||
| uint64_t actual_code = ((uint64_t*)(run->mmio.data))[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.
This is where uint64_t should be replaced with uint64.
| r0 = openat$kvm(0, &AUTO='/dev/kvm\x00', 0x0, 0x0) | ||
| r1 = ioctl$KVM_CREATE_VM(r0, AUTO, 0x0) | ||
| r2 = syz_kvm_setup_syzos_vm$riscv64(r1, &(0x7f0000c00000/0x400000)=nil) | ||
|
|
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.
For this test, and the following one, can you please add a line or two describing what they are supposed to test?
| } | ||
|
|
||
| # Table 5 in https://docs.riscv.org/reference/isa/_attachments/riscv-privileged.pdf . | ||
| riscv64_csr = 0x100, 0x104, 0x105, 0x140, 0x141, 0x142, 0x143, 0x144, 0x180, 0x106, 0x10a |
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 are 18 registers in Table 5, why aren't you including all of them?
(Maybe what you are doing is right, but please explain that in the comment)
| 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.
SYZOS guest functions are meant to be mutable via syz_kvm_add_vcpu parameters
I don't understand. All of SYZOS code is immutable, we can only pass parameters to it.
What is the purpose of guest_unexp_trap(), is it something like a default interrupt handler?
In that case, it is probably meant to live in SYZOS.
Also, right now these functions are not marked as GUEST_CODE. If e.g. guest_unexp_trap() happens to call sbi_ecall() instead of inlining it, everything will break, because sbi_ecall() won't be in the guest address space.
| // implementation in Linux kselftest KVM RISC-V tests. | ||
| // See https://elixir.bootlin.com/linux/v6.19-rc5/source/tools/testing/selftests/kvm/lib/riscv/processor.c#L337 . | ||
|
|
||
| #define KVM_RISCV_SELFTESTS_SBI_EXT 0x08FFFFFF |
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 remove "SELFTESTS" from the name.
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