diff options
| author | Dmitry Vyukov <dvyukov@google.com> | 2024-06-28 16:33:04 +0200 |
|---|---|---|
| committer | Dmitry Vyukov <dvyukov@google.com> | 2024-07-01 13:48:43 +0000 |
| commit | a6f99ace4014896f81a2f101416fd5413579f2bd (patch) | |
| tree | c6ace6c5a8736261fd462e83e19bbb88bd1a2ee3 /executor | |
| parent | 1f0ee43044bc8fc00bc1eccc85a93bf2b9972dd1 (diff) | |
pkg/rpcserver: move kernel test/data range checks from executor
We see some errors of the form:
SYZFAIL: coverage filter is full
pc=0x80007000c0008 regions=[0xffffffffbfffffff 0x243fffffff 0x143fffffff 0xc3fffffff] alloc=156
Executor shouldn't send non kernel addresses in signal,
but somehow it does. It can happen if the VM memory is corrupted,
or if the test program does something very nasty (e.g. discovers
the output region and writes to it).
It's not possible to reliably filter signal in the tested VM.
Move all of the filtering logic to the host.
Fixes #4942
Diffstat (limited to 'executor')
| -rw-r--r-- | executor/common_test.h | 2 | ||||
| -rw-r--r-- | executor/executor.cc | 83 | ||||
| -rw-r--r-- | executor/executor_bsd.h | 15 | ||||
| -rw-r--r-- | executor/executor_darwin.h | 15 | ||||
| -rw-r--r-- | executor/executor_linux.h | 78 | ||||
| -rw-r--r-- | executor/executor_runner.h | 28 | ||||
| -rw-r--r-- | executor/executor_test.h | 63 | ||||
| -rw-r--r-- | executor/nocover.h | 15 |
8 files changed, 80 insertions, 219 deletions
diff --git a/executor/common_test.h b/executor/common_test.h index d580ae2ff..dc39fa326 100644 --- a/executor/common_test.h +++ b/executor/common_test.h @@ -170,7 +170,7 @@ static long syz_test_fuzzer1(volatile long a, volatile long b, volatile long c) #endif #if SYZ_EXECUTOR || __NR_syz_inject_cover -static long syz_inject_cover(volatile long a, volatile long b, volatile long c) +static long syz_inject_cover(volatile long a, volatile long b) #if SYZ_EXECUTOR ; // defined in executor_test.h #else diff --git a/executor/executor.cc b/executor/executor.cc index 616e86752..fb5d242de 100644 --- a/executor/executor.cc +++ b/executor/executor.cc @@ -314,7 +314,8 @@ const uint64 no_copyout = -1; static int running; static uint32 completed; -static bool is_kernel_64_bit = true; +static bool is_kernel_64_bit; +static bool use_cover_edges; static uint8* input_data; @@ -395,20 +396,20 @@ const uint64 kInMagic = 0xbadc0ffeebadface; struct handshake_req { uint64 magic; + bool use_cover_edges; + bool is_kernel_64_bit; rpc::ExecEnv flags; uint64 pid; uint64 sandbox_arg; + uint64 syscall_timeout_ms; + uint64 program_timeout_ms; + uint64 slowdown_scale; }; struct execute_req { uint64 magic; uint64 id; - rpc::ExecEnv env_flags; uint64 exec_flags; - uint64 pid; - uint64 syscall_timeout_ms; - uint64 program_timeout_ms; - uint64 slowdown_scale; uint64 all_call_signal; bool all_extra_signal; }; @@ -681,28 +682,6 @@ void setup_control_pipes() fail("dup2(2, 0) failed"); } -void parse_env_flags(rpc::ExecEnv flags) -{ - // Note: Values correspond to ordering in pkg/ipc/ipc.go, e.g. FlagSandboxNamespace - flag_debug = (bool)(flags & rpc::ExecEnv::Debug); - flag_coverage = (bool)(flags & rpc::ExecEnv::Signal); - flag_sandbox_none = (bool)(flags & rpc::ExecEnv::SandboxNone); - flag_sandbox_setuid = (bool)(flags & rpc::ExecEnv::SandboxSetuid); - flag_sandbox_namespace = (bool)(flags & rpc::ExecEnv::SandboxNamespace); - flag_sandbox_android = (bool)(flags & rpc::ExecEnv::SandboxAndroid); - flag_extra_coverage = (bool)(flags & rpc::ExecEnv::ExtraCover); - flag_net_injection = (bool)(flags & rpc::ExecEnv::EnableTun); - flag_net_devices = (bool)(flags & rpc::ExecEnv::EnableNetDev); - flag_net_reset = (bool)(flags & rpc::ExecEnv::EnableNetReset); - flag_cgroups = (bool)(flags & rpc::ExecEnv::EnableCgroups); - flag_close_fds = (bool)(flags & rpc::ExecEnv::EnableCloseFds); - flag_devlink_pci = (bool)(flags & rpc::ExecEnv::EnableDevlinkPCI); - flag_vhci_injection = (bool)(flags & rpc::ExecEnv::EnableVhciInjection); - flag_wifi = (bool)(flags & rpc::ExecEnv::EnableWifi); - flag_delay_kcov_mmap = (bool)(flags & rpc::ExecEnv::DelayKcovMmap); - flag_nic_vf = (bool)(flags & rpc::ExecEnv::EnableNicVF); -} - void receive_handshake() { handshake_req req = {}; @@ -714,27 +693,40 @@ void receive_handshake() #if SYZ_HAVE_SANDBOX_ANDROID sandbox_arg = req.sandbox_arg; #endif - parse_env_flags(req.flags); + is_kernel_64_bit = req.is_kernel_64_bit; + use_cover_edges = req.use_cover_edges; procid = req.pid; + syscall_timeout_ms = req.syscall_timeout_ms; + program_timeout_ms = req.program_timeout_ms; + slowdown_scale = req.slowdown_scale; + flag_debug = (bool)(req.flags & rpc::ExecEnv::Debug); + flag_coverage = (bool)(req.flags & rpc::ExecEnv::Signal); + flag_sandbox_none = (bool)(req.flags & rpc::ExecEnv::SandboxNone); + flag_sandbox_setuid = (bool)(req.flags & rpc::ExecEnv::SandboxSetuid); + flag_sandbox_namespace = (bool)(req.flags & rpc::ExecEnv::SandboxNamespace); + flag_sandbox_android = (bool)(req.flags & rpc::ExecEnv::SandboxAndroid); + flag_extra_coverage = (bool)(req.flags & rpc::ExecEnv::ExtraCover); + flag_net_injection = (bool)(req.flags & rpc::ExecEnv::EnableTun); + flag_net_devices = (bool)(req.flags & rpc::ExecEnv::EnableNetDev); + flag_net_reset = (bool)(req.flags & rpc::ExecEnv::EnableNetReset); + flag_cgroups = (bool)(req.flags & rpc::ExecEnv::EnableCgroups); + flag_close_fds = (bool)(req.flags & rpc::ExecEnv::EnableCloseFds); + flag_devlink_pci = (bool)(req.flags & rpc::ExecEnv::EnableDevlinkPCI); + flag_vhci_injection = (bool)(req.flags & rpc::ExecEnv::EnableVhciInjection); + flag_wifi = (bool)(req.flags & rpc::ExecEnv::EnableWifi); + flag_delay_kcov_mmap = (bool)(req.flags & rpc::ExecEnv::DelayKcovMmap); + flag_nic_vf = (bool)(req.flags & rpc::ExecEnv::EnableNicVF); } -static execute_req last_execute_req; - void receive_execute() { - execute_req& req = last_execute_req; + execute_req req = {}; ssize_t n = read(kInPipeFd, &req, sizeof(req)); if (n != (ssize_t)sizeof(req)) failmsg("control pipe read failed", "read=%zd want=%zd", n, sizeof(req)); if (req.magic != kInMagic) failmsg("bad execute request magic", "magic=0x%llx", req.magic); request_id = req.id; - parse_env_flags(req.env_flags); - procid = req.pid; - request_id = req.id; - syscall_timeout_ms = req.syscall_timeout_ms; - program_timeout_ms = req.program_timeout_ms; - slowdown_scale = req.slowdown_scale; flag_collect_signal = req.exec_flags & (1 << 0); flag_collect_cover = req.exec_flags & (1 << 1); flag_dedup_cover = req.exec_flags & (1 << 2); @@ -744,10 +736,11 @@ void receive_execute() all_extra_signal = req.all_extra_signal; debug("[%llums] exec opts: procid=%llu threaded=%d cover=%d comps=%d dedup=%d signal=%d " - " sandbox=%d/%d/%d/%d timeouts=%llu/%llu/%llu\n", + " sandbox=%d/%d/%d/%d timeouts=%llu/%llu/%llu kernel_64_bit=%d\n", current_time_ms() - start_time_ms, procid, flag_threaded, flag_collect_cover, flag_comparisons, flag_dedup_cover, flag_collect_signal, flag_sandbox_none, flag_sandbox_setuid, - flag_sandbox_namespace, flag_sandbox_android, syscall_timeout_ms, program_timeout_ms, slowdown_scale); + flag_sandbox_namespace, flag_sandbox_android, syscall_timeout_ms, program_timeout_ms, slowdown_scale, + is_kernel_64_bit); if (syscall_timeout_ms == 0 || program_timeout_ms <= syscall_timeout_ms || slowdown_scale == 0) failmsg("bad timeouts", "syscall=%llu, program=%llu, scale=%llu", syscall_timeout_ms, program_timeout_ms, slowdown_scale); @@ -1055,10 +1048,8 @@ uint32 write_signal(flatbuffers::FlatBufferBuilder& fbb, cover_t* cov, bool all) bool prev_filter = true; for (uint32 i = 0; i < cov->size; i++) { cover_data_t pc = cover_data[i] + cov->pc_offset; - if (is_kernel_pc(pc) < 0) - exitf("got bad pc: 0x%llx", (uint64)pc); uint64 sig = pc; - if (use_cover_edges(pc)) { + if (use_cover_edges) { // Only hash the lower 12 bits so the hash is independent of any module offsets. const uint64 mask = (1 << 12) - 1; sig ^= hash(prev_pc & mask) & mask; @@ -1630,12 +1621,6 @@ std::tuple<rpc::ComparisonRaw, bool, bool> convert(const kcov_comparison_t& cmp) return {}; if (arg2 >= out_start && arg2 <= out_end) return {}; - // Filter out kernel physical memory addresses. - // These are internal kernel comparisons and should not be interesting. - bool kptr1 = is_kernel_data(arg1) || is_kernel_pc(arg1) > 0 || arg1 == 0; - bool kptr2 = is_kernel_data(arg2) || is_kernel_pc(arg2) > 0 || arg2 == 0; - if (kptr1 && kptr2) - return {}; if (!coverage_filter(cmp.pc)) return {}; diff --git a/executor/executor_bsd.h b/executor/executor_bsd.h index 3ee4be80c..e8530f614 100644 --- a/executor/executor_bsd.h +++ b/executor/executor_bsd.h @@ -179,21 +179,6 @@ static void cover_collect(cover_t* cov) cov->size = *(uint64*)cov->data; } -static bool is_kernel_data(uint64 addr) -{ - return false; -} - -static int is_kernel_pc(uint64 pc) -{ - return 0; -} - -static bool use_cover_edges(uint64 pc) -{ - return true; -} - #if GOOS_netbsd #define SYZ_HAVE_FEATURES 1 static feature_t features[] = { diff --git a/executor/executor_darwin.h b/executor/executor_darwin.h index 11146acc3..76b939fcf 100644 --- a/executor/executor_darwin.h +++ b/executor/executor_darwin.h @@ -121,18 +121,3 @@ static void cover_collect(cover_t* cov) cov->data_offset = ((int64_t) & (trace->pcs)) - ((int64_t)(cov->data)); cov->pc_offset = trace->offset; } - -static bool is_kernel_data(uint64 addr) -{ - return false; -} - -static int is_kernel_pc(uint64 pc) -{ - return 0; -} - -static bool use_cover_edges(uint64 pc) -{ - return true; -} diff --git a/executor/executor_linux.h b/executor/executor_linux.h index cb980838f..ff8f3bc60 100644 --- a/executor/executor_linux.h +++ b/executor/executor_linux.h @@ -36,8 +36,6 @@ struct kcov_remote_arg { #define KCOV_SUBSYSTEM_MASK (0xffull << 56) #define KCOV_INSTANCE_MASK (0xffffffffull) -static bool is_gvisor; - static inline __u64 kcov_remote_handle(__u64 subsys, __u64 inst) { if (subsys & ~KCOV_SUBSYSTEM_MASK || inst & ~KCOV_INSTANCE_MASK) @@ -45,14 +43,9 @@ static inline __u64 kcov_remote_handle(__u64 subsys, __u64 inst) return subsys | inst; } -static bool detect_kernel_bitness(); -static bool detect_gvisor(); - static void os_init(int argc, char** argv, char* data, size_t data_size) { prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0); - is_kernel_64_bit = detect_kernel_bitness(); - is_gvisor = detect_gvisor(); // Surround the main data mapping with PROT_NONE pages to make virtual address layout more consistent // across different configurations (static/non-static build) and C repros. // One observed case before: executor had a mapping above the data mapping (output region), @@ -177,77 +170,6 @@ static void cover_collect(cover_t* cov) cov->size = *(uint32*)cov->data; } -static bool use_cover_edges(uint32 pc) -{ - return true; -} - -static bool is_kernel_data(uint64 addr) -{ - if (is_gvisor) - return false; -#if GOARCH_386 || GOARCH_amd64 - // This range corresponds to the first 1TB of the physical memory mapping, - // see Documentation/arch/x86/x86_64/mm.rst. - return addr >= 0xffff880000000000ull && addr < 0xffff890000000000ull; -#else - return false; -#endif -} - -// Returns >0 for yes, <0 for no, 0 for don't know. -static int is_kernel_pc(uint64 pc) -{ - if (is_gvisor) - return 0; -#if GOARCH_386 || GOARCH_amd64 - // Text/modules range for x86_64. - return pc >= 0xffffffff80000000ull && pc < 0xffffffffff000000ull ? 1 : -1; -#else - return 0; -#endif -} - -static bool use_cover_edges(uint64 pc) -{ -#if GOARCH_amd64 || GOARCH_arm64 - if (is_gvisor) - return false; // gvisor coverage is not a trace, so producing edges won't work -#endif - return true; -} - -static bool detect_kernel_bitness() -{ - if (sizeof(void*) == 8) - return true; - // It turns out to be surprisingly hard to understand if the kernel underneath is 64-bits. - // A common method is to look at uname.machine. But it is produced in some involved ways, - // and we will need to know about all strings it returns and in the end it can be overriden - // during build and lie (and there are known precedents of this). - // So instead we look at size of addresses in /proc/kallsyms. - bool wide = true; - int fd = open("/proc/kallsyms", O_RDONLY); - if (fd != -1) { - char buf[16]; - if (read(fd, buf, sizeof(buf)) == sizeof(buf) && - (buf[8] == ' ' || buf[8] == '\t')) - wide = false; - close(fd); - } - debug("detected %d-bit kernel\n", wide ? 64 : 32); - return wide; -} - -static bool detect_gvisor() -{ - char buf[64] = {}; - // 3 stands for undeclared SYSLOG_ACTION_READ_ALL. - syscall(__NR_syslog, 3, buf, sizeof(buf) - 1); - // This is a first line of gvisor dmesg. - return strstr(buf, "Starting gVisor"); -} - // One does not simply exit. // _exit can in fact fail. // syzkaller did manage to generate a seccomp filter that prohibits exit_group syscall. diff --git a/executor/executor_runner.h b/executor/executor_runner.h index 100ed87f6..86ce1819a 100644 --- a/executor/executor_runner.h +++ b/executor/executor_runner.h @@ -33,12 +33,14 @@ class Proc { public: Proc(Connection& conn, const char* bin, int id, int max_signal_fd, int cover_filter_fd, - uint32 slowdown, uint32 syscall_timeout_ms, uint32 program_timeout_ms) + bool use_cover_edges, bool is_kernel_64_bit, uint32 slowdown, uint32 syscall_timeout_ms, uint32 program_timeout_ms) : conn_(conn), bin_(bin), id_(id), max_signal_fd_(max_signal_fd), cover_filter_fd_(cover_filter_fd), + use_cover_edges_(use_cover_edges), + is_kernel_64_bit_(is_kernel_64_bit), slowdown_(slowdown), syscall_timeout_ms_(syscall_timeout_ms), program_timeout_ms_(program_timeout_ms), @@ -129,6 +131,8 @@ private: const int id_; const int max_signal_fd_; const int cover_filter_fd_; + const bool use_cover_edges_; + const bool is_kernel_64_bit_; const uint32 slowdown_; const uint32 syscall_timeout_ms_; const uint32 program_timeout_ms_; @@ -265,9 +269,14 @@ private: sandbox_arg_ = msg_->exec_opts->sandbox_arg(); handshake_req req = { .magic = kInMagic, + .use_cover_edges = use_cover_edges_, + .is_kernel_64_bit = is_kernel_64_bit_, .flags = exec_env_, .pid = static_cast<uint64>(id_), .sandbox_arg = static_cast<uint64>(sandbox_arg_), + .syscall_timeout_ms = syscall_timeout_ms_, + .program_timeout_ms = program_timeout_ms_, + .slowdown_scale = slowdown_, }; if (write(req_pipe_, &req, sizeof(req)) != sizeof(req)) { debug("request pipe write failed (errno=%d)\n", errno); @@ -312,12 +321,7 @@ private: execute_req req{ .magic = kInMagic, .id = static_cast<uint64>(msg_->id), - .env_flags = exec_env_, .exec_flags = static_cast<uint64>(msg_->exec_opts->exec_flags()), - .pid = static_cast<uint64>(id_), - .syscall_timeout_ms = syscall_timeout_ms_, - .program_timeout_ms = program_timeout_ms_, - .slowdown_scale = slowdown_, .all_call_signal = all_call_signal, .all_extra_signal = all_extra_signal, }; @@ -461,7 +465,7 @@ public: int cover_filter_fd = cover_filter_ ? cover_filter_->FD() : -1; for (size_t i = 0; i < num_procs; i++) procs_.emplace_back(new Proc(conn, bin, i, max_signal_fd, cover_filter_fd, - slowdown_, syscall_timeout_ms_, program_timeout_ms_)); + use_cover_edges_, is_kernel_64_bit_, slowdown_, syscall_timeout_ms_, program_timeout_ms_)); for (;;) Loop(); @@ -475,6 +479,8 @@ private: std::vector<std::unique_ptr<Proc>> procs_; std::deque<rpc::ExecRequestRawT> requests_; std::vector<std::string> leak_frames_; + bool use_cover_edges_ = false; + bool is_kernel_64_bit_ = false; uint32 slowdown_ = 0; uint32 syscall_timeout_ms_ = 0; uint32 program_timeout_ms_ = 0; @@ -538,11 +544,14 @@ private: conn_.Recv(conn_reply); if (conn_reply.debug) flag_debug = true; - debug("connected to manager: procs=%d slowdown=%d syscall_timeout=%u" + debug("connected to manager: procs=%d cover_edges=%d kernel_64_bit=%d slowdown=%d syscall_timeout=%u" " program_timeout=%u features=0x%llx\n", - conn_reply.procs, conn_reply.slowdown, conn_reply.syscall_timeout_ms, + conn_reply.procs, conn_reply.cover_edges, conn_reply.kernel_64_bit, + conn_reply.slowdown, conn_reply.syscall_timeout_ms, conn_reply.program_timeout_ms, static_cast<uint64>(conn_reply.features)); leak_frames_ = conn_reply.leak_frames; + use_cover_edges_ = conn_reply.cover_edges; + is_kernel_64_bit_ = is_kernel_64_bit = conn_reply.kernel_64_bit; slowdown_ = conn_reply.slowdown; syscall_timeout_ms_ = conn_reply.syscall_timeout_ms; program_timeout_ms_ = conn_reply.program_timeout_ms; @@ -555,7 +564,6 @@ private: // This does any one-time setup for the requested features on the machine. // Note: this can be called multiple times and must be idempotent. - // is_kernel_64_bit = detect_kernel_bitness(); #if SYZ_HAVE_FEATURES setup_sysctl(); setup_cgroups(); diff --git a/executor/executor_test.h b/executor/executor_test.h index c2802dd2a..5e128d851 100644 --- a/executor/executor_test.h +++ b/executor/executor_test.h @@ -9,6 +9,7 @@ #include <sys/prctl.h> #endif +// sys/targets also know about these consts. static uint64 kernel_text_start = 0xc0dec0dec0000000; static uint64 kernel_text_mask = 0xffffff; @@ -35,17 +36,30 @@ static void os_init(int argc, char** argv, void* data, size_t data_size) extern "C" notrace void __sanitizer_cov_trace_pc(void) { - unsigned long ip = (unsigned long)__builtin_return_address(0); - // Convert to what is_kernel_pc will accept as valid coverage; - ip = kernel_text_start | (ip & kernel_text_mask); if (current_thread == nullptr || current_thread->cov.data == nullptr || current_thread->cov.collect_comps) return; - unsigned long* start = (unsigned long*)current_thread->cov.data; - unsigned long* end = (unsigned long*)current_thread->cov.data_end; - int pos = start[0]; - if (start + pos + 1 < end) { - start[0] = pos + 1; - start[pos + 1] = ip; + uint64 pc = (uint64)__builtin_return_address(0); + // Convert to what is_kernel_pc will accept as valid coverage; + pc = kernel_text_start | (pc & kernel_text_mask); + // Note: we duplicate the following code instead of using a template function + // because it must not be instrumented which is hard to achieve for all compiler + // if the code is in a separate function. + if (is_kernel_64_bit) { + uint64* start = (uint64*)current_thread->cov.data; + uint64* end = (uint64*)current_thread->cov.data_end; + uint64 pos = start[0]; + if (start + pos + 1 < end) { + start[0] = pos + 1; + start[pos + 1] = pc; + } + } else { + uint32* start = (uint32*)current_thread->cov.data; + uint32* end = (uint32*)current_thread->cov.data_end; + uint32 pos = start[0]; + if (start + pos + 1 < end) { + start[0] = pos + 1; + start[pos + 1] = pc; + } } } @@ -97,7 +111,7 @@ static void cover_mmap(cover_t* cov) if (cov->data == MAP_FAILED) exitf("cover mmap failed"); cov->data_end = cov->data + cov->mmap_alloc_size; - cov->data_offset = sizeof(unsigned long); + cov->data_offset = is_kernel_64_bit ? sizeof(uint64_t) : sizeof(uint32_t); // We don't care about the specific PC values for now. // Once we do, we might want to consider ASLR here. cov->pc_offset = 0; @@ -107,36 +121,13 @@ static void cover_unprotect(cover_t* cov) { } -static bool is_kernel_data(uint64 addr) -{ - return addr >= 0xda1a0000 && addr <= 0xda1a1000; -} - -static int is_kernel_pc(uint64 pc) -{ - uint64 start = kernel_text_start; - uint64 end = kernel_text_start | kernel_text_mask; - if (!is_kernel_64_bit) { - start = (uint32)start; - end = (uint32)end; - } - return pc >= start && pc <= end ? 1 : -1; -} - -static bool use_cover_edges(uint64 pc) -{ - return true; -} - -static long syz_inject_cover(volatile long a, volatile long b, volatile long c) +static long syz_inject_cover(volatile long a, volatile long b) { cover_t* cov = ¤t_thread->cov; if (cov->data == nullptr) return ENOENT; - is_kernel_64_bit = a; - cov->data_offset = is_kernel_64_bit ? sizeof(uint64_t) : sizeof(uint32_t); - uint32 size = std::min((uint32)c, cov->mmap_alloc_size); - memcpy(cov->data, (void*)b, size); + uint32 size = std::min((uint32)b, cov->mmap_alloc_size); + memcpy(cov->data, (void*)a, size); memset(cov->data + size, 0xcd, std::min<uint64>(100, cov->mmap_alloc_size - size)); return 0; } diff --git a/executor/nocover.h b/executor/nocover.h index b097e9f43..10e256cdd 100644 --- a/executor/nocover.h +++ b/executor/nocover.h @@ -28,18 +28,3 @@ static void cover_mmap(cover_t* cov) static void cover_unprotect(cover_t* cov) { } - -static bool is_kernel_data(uint64 addr) -{ - return false; -} - -static int is_kernel_pc(uint64 pc) -{ - return 0; -} - -static bool use_cover_edges(uint64 pc) -{ - return true; -} |
