From 1730a161946e10172c8e52c5dbddf20812aa0deb Mon Sep 17 00:00:00 2001 From: Jordan Rome Date: Tue, 7 May 2024 10:16:24 -0400 Subject: [PATCH] Don't unpack kernel headers or look in tmp Looking in shared writeable locations for kernel headers is inherently risky even bpftrace does the unpacking. Remove this functionality and let the user specify the path to these headers if we can't find them in known locations. References: https://github.com/bpftrace/bpftrace/pull/3033 https://github.com/bpftrace/bpftrace/pull/3154 --- src/clang_parser.cpp | 3 -- src/fuzz_main.cpp | 2 +- src/main.cpp | 40 ++++++++++----- src/utils.cpp | 115 ++++--------------------------------------- src/utils.h | 3 +- tests/utils.cpp | 21 -------- 6 files changed, 38 insertions(+), 146 deletions(-) diff --git a/src/clang_parser.cpp b/src/clang_parser.cpp index d2088e49ae78..66d022dfab07 100644 --- a/src/clang_parser.cpp +++ b/src/clang_parser.cpp @@ -660,9 +660,6 @@ bool ClangParser::parse(ast::Program *program, StderrSilencer silencer; silencer.silence(); #endif - if (program->c_definitions.empty() && bpftrace.btf_set_.empty()) - return true; - input = "#include \n" + program->c_definitions; diff --git a/src/fuzz_main.cpp b/src/fuzz_main.cpp index 5ebcf3e30736..4a1d49101d5c 100644 --- a/src/fuzz_main.cpp +++ b/src/fuzz_main.cpp @@ -131,7 +131,7 @@ int fuzz_main(const char* data, size_t sz) struct utsname utsname; uname(&utsname); std::string ksrc, kobj; - auto kdirs = get_kernel_dirs(utsname, !bpftrace.feature_->has_btf()); + auto kdirs = get_kernel_dirs(utsname); ksrc = std::get<0>(kdirs); kobj = std::get<1>(kdirs); diff --git a/src/main.cpp b/src/main.cpp index 527a8a781925..1962cb956f43 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -389,18 +389,6 @@ static void parse_env(BPFtrace& bpftrace) ClangParser clang; std::vector extra_flags; - { - struct utsname utsname; - uname(&utsname); - std::string ksrc, kobj; - auto kdirs = get_kernel_dirs(utsname); - ksrc = std::get<0>(kdirs); - kobj = std::get<1>(kdirs); - - if (ksrc != "") - extra_flags = get_kernel_cflags( - utsname.machine, ksrc, kobj, bpftrace.kconfig); - } extra_flags.push_back("-include"); extra_flags.push_back("/bpftrace/include/" CLANG_WORKAROUNDS_H); @@ -420,7 +408,33 @@ static void parse_env(BPFtrace& bpftrace) if (!include_files.empty() && driver.root->c_definitions.empty()) driver.root->c_definitions = "#define __BPFTRACE_DUMMY__"; - if (!clang.parse(driver.root.get(), bpftrace, extra_flags)) + bool should_clang_parse = !driver.root.get()->c_definitions.empty() || + !bpftrace.btf_set_.empty(); + + { + struct utsname utsname; + uname(&utsname); + std::string ksrc, kobj; + if (should_clang_parse) { + auto kdirs = get_kernel_dirs(utsname); + ksrc = std::get<0>(kdirs); + kobj = std::get<1>(kdirs); + } else { + ksrc = ""; + kobj = ""; + } + + if (ksrc != "") { + std::vector kernel_cflags = get_kernel_cflags( + utsname.machine, ksrc, kobj, bpftrace.kconfig); + extra_flags.insert(extra_flags.end(), + std::make_move_iterator(kernel_cflags.begin()), + std::make_move_iterator(kernel_cflags.end())); + } + } + + if (should_clang_parse && + !clang.parse(driver.root.get(), bpftrace, extra_flags)) return nullptr; err = driver.parse(); diff --git a/src/utils.cpp b/src/utils.cpp index aae2d8891039..8ebcb4de9985 100644 --- a/src/utils.cpp +++ b/src/utils.cpp @@ -108,8 +108,6 @@ const struct vmlinux_location vmlinux_locs[] = { { nullptr, false }, }; -constexpr std::string_view PROC_KHEADERS_PATH = "/sys/kernel/kheaders.tar.xz"; - static bool pid_in_different_mountns(int pid); static std::vector resolve_binary_path(const std::string &cmd, const char *env_paths, @@ -683,103 +681,6 @@ bool is_dir(const std::string &path) return std_filesystem::is_directory(buf, ec); } -bool file_exists_and_ownedby_root(const char *f) -{ - struct stat st; - if (stat(f, &st) == 0) { - if (st.st_uid != 0) { - LOG(ERROR) << "header file ownership expected to be root: " - << std::string(f); - return false; - } - return true; - } - return false; -} - -namespace { -struct KernelHeaderTmpDir { - KernelHeaderTmpDir(const std::string &prefix) : path{ prefix + "XXXXXX" } - { - if (::mkdtemp(&path[0]) == nullptr) { - LOG(FATAL) << "creating temporary path for kheaders.tar.xz failed"; - } - } - - ~KernelHeaderTmpDir() - { - if (path.size() > 0) { - // move_to either did not succeed or did not run, so clean up after - // ourselves - exec_system(("rm -rf " + path).c_str()); - } - } - - void move_to(const std::string &new_path) - { - int err = ::rename(path.c_str(), new_path.c_str()); - if (err == 0) { - path = ""; - } - } - - std::string path; -}; - -std::string unpack_kheaders_tar_xz(const struct utsname &utsname) -{ - std::error_code ec; -#if defined(__ANDROID__) - std_filesystem::path path_prefix{ "/data/local/tmp" }; -#else - std_filesystem::path path_prefix{ "/tmp" }; -#endif - std_filesystem::path path_kheaders{ PROC_KHEADERS_PATH }; - if (const char *tmpdir = ::getenv("TMPDIR")) { - path_prefix = tmpdir; - } - path_prefix /= "kheaders-"; - std_filesystem::path shared_path{ path_prefix.string() + utsname.release }; - - if (file_exists_and_ownedby_root(shared_path.c_str())) { - // already unpacked - return shared_path.string(); - } - - if (!std_filesystem::exists(path_kheaders, ec)) { - StderrSilencer silencer; - silencer.silence(); - - FILE *modprobe = ::popen("modprobe kheaders", "w"); - if (modprobe == nullptr || pclose(modprobe) != 0) { - return ""; - } - - if (!std_filesystem::exists(path_kheaders, ec)) { - return ""; - } - } - - KernelHeaderTmpDir tmpdir{ path_prefix }; - - FILE *tar = ::popen(("tar xf " + std::string(PROC_KHEADERS_PATH) + " -C " + - tmpdir.path) - .c_str(), - "w"); - if (!tar) { - return ""; - } - - int rc = ::pclose(tar); - if (rc == 0) { - tmpdir.move_to(shared_path); - return shared_path; - } - - return ""; -} -} // namespace - // get_kernel_dirs returns {ksrc, kobj} - directories for pristine and // generated kernel sources. // @@ -798,8 +699,7 @@ std::string unpack_kheaders_tar_xz(const struct utsname &utsname) // Both ksrc and kobj are guaranteed to be != "", if at least some trace of // kernel sources was found. std::tuple get_kernel_dirs( - const struct utsname &utsname, - bool unpack_kheaders) + const struct utsname &utsname) { #ifdef KERNEL_HEADERS_DIR return { KERNEL_HEADERS_DIR, KERNEL_HEADERS_DIR }; @@ -827,11 +727,14 @@ std::tuple get_kernel_dirs( kobj = ""; } if (ksrc.empty() && kobj.empty()) { - if (unpack_kheaders) { - const auto kheaders_tar_xz_path = unpack_kheaders_tar_xz(utsname); - if (kheaders_tar_xz_path.size() > 0) - return std::make_tuple(kheaders_tar_xz_path, kheaders_tar_xz_path); - } + LOG(WARNING) << "Could not find kernel headers in " << ksrc << " or " + << kobj + << ". To specify a particular path to kernel headers, set the " + "env variables BPFTRACE_KERNEL_SOURCE and, optionally, " + "BPFTRACE_KERNEL_BUILD if the kernel was built in a " + "different directory than its source. To create kernel " + "headers run 'modprobe kheaders', which will create a tar " + "file at /sys/kernel/kheaders.tar.xz"; return std::make_tuple("", ""); } if (ksrc.empty()) { diff --git a/src/utils.h b/src/utils.h index 6ee19c1e0b96..64eca35d5e9b 100644 --- a/src/utils.h +++ b/src/utils.h @@ -160,8 +160,7 @@ std::vector get_possible_cpus(); bool is_dir(const std::string &path); bool file_exists_and_ownedby_root(const char *f); std::tuple get_kernel_dirs( - const struct utsname &utsname, - bool unpack_kheaders = true); + const struct utsname &utsname); std::vector get_kernel_cflags(const char *uname_machine, const std::string &ksrc, const std::string &kobj, diff --git a/tests/utils.cpp b/tests/utils.cpp index e996f5a458f8..d5f8a2d760a8 100644 --- a/tests/utils.cpp +++ b/tests/utils.cpp @@ -358,27 +358,6 @@ TEST(utils, get_pids_for_program) ASSERT_EQ(pids.size(), 0); } -TEST(utils, file_exists_and_ownedby_root) -{ - std::string tmpdir = "/tmp/bpftrace-test-utils-XXXXXX"; - std::string file1 = "/ownedby-user"; - std::string file2 = "/no-exists"; - if (::mkdtemp(tmpdir.data()) == nullptr) { - throw std::runtime_error("creating temporary path for tests failed"); - } - - int fd; - fd = open((tmpdir + file1).c_str(), O_CREAT, S_IRUSR); - close(fd); - ASSERT_GE(fd, 0); - - EXPECT_FALSE(file_exists_and_ownedby_root((tmpdir + file1).c_str())); - EXPECT_FALSE(file_exists_and_ownedby_root((tmpdir + file2).c_str())); - EXPECT_TRUE(file_exists_and_ownedby_root("/proc/1/maps")); - - EXPECT_GT(std_filesystem::remove_all(tmpdir), 0); -} - } // namespace utils } // namespace test } // namespace bpftrace