Use symcheck to locate writeable+executable object files

ci/run.sh

@@ -50,6 +50,9 @@ SYMCHECK_TEST_TARGET="$target" cargo test -p symbol-check --release
 symcheck=(cargo run -p symbol-check --release)
 symcheck+=(-- --build-and-check --target "$target")
 
+# Executable section checks are meaningless on no-std targets
+[[ "$target" == *"-none"* ]] && symcheck+=(--no-kernel)
+
 "${symcheck[@]}" -- -p compiler_builtins
 "${symcheck[@]}" -- -p compiler_builtins --release
 "${symcheck[@]}" -- -p compiler_builtins --features c

crates/symbol-check/src/main.rs

@@ -5,26 +5,27 @@
 //! actual target is cross compiled.
 
 use std::collections::{BTreeMap, BTreeSet, HashSet};
-use std::fs;
 use std::io::{BufRead, BufReader};
 use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio, exit};
 use std::sync::LazyLock;
+use std::{env, fs};
 
 use object::read::archive::ArchiveFile;
 use object::{
-    File as ObjFile, Object, ObjectSection, ObjectSymbol, Result as ObjResult, Symbol, SymbolKind,
-    SymbolScope,
+    Architecture, BinaryFormat, File as ObjFile, Object, ObjectSection, ObjectSymbol,
+    Result as ObjResult, SectionFlags, Symbol, SymbolKind, SymbolScope, elf,
 };
 use regex::Regex;
 use serde_json::Value;
 
 const CHECK_LIBRARIES: &[&str] = &["compiler_builtins", "builtins_test_intrinsics"];
 const CHECK_EXTENSIONS: &[Option<&str>] = &[Some("rlib"), Some("a"), Some("exe"), None];
+const GNU_STACK: &str = ".note.GNU-stack";
 
 const USAGE: &str = "Usage:
 
-    symbol-check --build-and-check [--target TARGET] -- CARGO_BUILD_ARGS ...
+    symbol-check --build-and-check [--target TARGET] [--no-kernel] -- CARGO_BUILD_ARGS ...
     symbol-check --check PATHS ...\
 ";
 
@@ -51,6 +52,12 @@ fn main() {
         "Run checks on the given set of paths, without invoking Cargo. Paths \
         may be either archives or object files.",
     );
+    opts.optflag(
+        "",
+        "no-kernel",
+        "The binaries will not be checked for executable stacks. Used for embedded targets which \
+        don't set `.note.GNU-stack` since there is no protection.",
+    );
 
     let print_usage_and_exit = |code: i32| -> ! {
         eprintln!("{}", opts.usage(USAGE));
@@ -66,57 +73,56 @@ fn main() {
         print_usage_and_exit(0);
     }
 
+    let verify_no_exe = !m.opt_present("no-kernel");
     let free_args = m.free.iter().map(String::as_str).collect::<Vec<_>>();
+    for arg in &free_args {
+        assert!(
+            !arg.contains("--target"),
+            "target must be passed to symbol-check"
+        );
+    }
 
     if m.opt_present("build-and-check") {
         let target = m.opt_str("target").unwrap_or(env!("HOST").to_string());
-        run_build_and_check(&target, &free_args);
+        let paths = exec_cargo_with_args(&target, &free_args);
+        check_paths(&paths, verify_no_exe);
     } else if m.opt_present("check") {
         if free_args.is_empty() {
             print_usage_and_exit(1);
         }
-        check_paths(&free_args);
+        check_paths(&free_args, verify_no_exe);
     } else {
         print_usage_and_exit(1);
     }
 }
 
-fn run_build_and_check(target: &str, args: &[&str]) {
-    // Make sure `--target` isn't passed to avoid confusion (since it should be
-    // proivded only once, positionally).
-    for arg in args {
-        assert!(
-            !arg.contains("--target"),
-            "target must be passed to symbol-check"
-        );
-    }
-
-    let paths = exec_cargo_with_args(target, args);
-    check_paths(&paths);
-}
-
-fn check_paths<P: AsRef<Path>>(paths: &[P]) {
+fn check_paths<P: AsRef<Path>>(paths: &[P], verify_no_exe: bool) {
     for path in paths {
         let path = path.as_ref();
         println!("Checking {}", path.display());
         let archive = BinFile::from_path(path);
 
         verify_no_duplicates(&archive);
         verify_core_symbols(&archive);
+        if verify_no_exe {
+            // We don't really have a good way of knowing whether or not an elf file is for a
+            // no-kernel environment, in which case note.GNU-stack doesn't get emitted.
+            verify_no_exec_stack(&archive);
+        }
     }
 }
 
 /// Run `cargo build` with the provided additional arguments, collecting the list of created
 /// libraries.
-fn exec_cargo_with_args(target: &str, args: &[&str]) -> Vec<PathBuf> {
+fn exec_cargo_with_args<S: AsRef<str>>(target: &str, args: &[S]) -> Vec<PathBuf> {
     let mut cmd = Command::new("cargo");
     cmd.args([
         "build",
         "--target",
         target,
         "--message-format=json-diagnostic-rendered-ansi",
     ])
-    .args(args)
+    .args(args.iter().map(|arg| arg.as_ref()))
     .stdout(Stdio::piped());
 
     println!("running: {cmd:?}");
@@ -323,6 +329,114 @@ fn verify_core_symbols(archive: &BinFile) {
     println!("    success: no undefined references to core found");
 }
 
+/// Ensure that the object/archive will not require an executable stack.
+fn verify_no_exec_stack(archive: &BinFile) {
+    let mut problem_objfiles = Vec::new();
+
+    archive.for_each_object(|obj, obj_path| {
+        let exe = obj_requires_exe_stack(&obj);
+        if !matches!(exe, ExeStack::None) {
+            problem_objfiles.push((obj_path.to_owned(), exe));
+        }
+    });
+
+    if !problem_objfiles.is_empty() {
+        eprintln!("the following object files require an executable stack:");
+        for (obj, exe) in problem_objfiles {
+            let reason = match exe {
+                ExeStack::None => unreachable!(),
+                ExeStack::NoGnuStackSection => "no .note.GNU-stack section",
+                ExeStack::ExeGnuStackSection => "executable .note.GNU-stack section",
+            };
+            eprintln!("    {obj} ({reason})");
+        }
+        exit(1);
+    }
+
+    println!("    success: no writeable+executable sections found");
+}
+
+enum ExeStack {
+    None,
+    NoGnuStackSection,
+    ExeGnuStackSection,
+}
+
+/// True if the section/flag combination indicates that the object file should be linked with an
+/// executable stack.
+///
+/// Paraphrased from <https://www.man7.org/linux/man-pages/man1/ld.1.html>:
+///
+/// - A `.note.GNU-stack` section with the exe flag means this needs an executable stack
+/// - A `.note.GNU-stack` section without the exe flag means there is no executable stack needed
+/// - Without the section, behavior is target-specific and on some targets means an executable
+///   stack is required.
+///
+/// If any object files meet the requirements for an executable stack, any final binary that links
+/// it will have a program header with a `PT_GNU_STACK` section, which will be marked `RWE` rather
+/// than the desired `RW`. (We don't check final binaries).
+///
+/// Per [1], it is now deprecated behavior for a missing `.note.GNU-stack` section to imply an
+/// executable stack. However, we shouldn't assume that tooling has caught up to this.
+///
+/// [1]: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0d38576a34ec64a1b4500c9277a8e9d0f07e6774>
+fn obj_requires_exe_stack(obj: &ObjFile) -> ExeStack {
+    // Files other than elf do not use the same convention.
+    if obj.format() != BinaryFormat::Elf {
+        return ExeStack::None;
+    }
+
+    let mut gnu_stack_exe = None;
+    let mut has_exe_sections = false;
+    for sec in obj.sections() {
+        dbg!(&sec);
+        let SectionFlags::Elf { sh_flags } = sec.flags() else {
+            unreachable!("only elf files are being checked");
+        };
+
+        let is_exe = (sh_flags & elf::SHF_EXECINSTR as u64) != 0;
+
+        // If the magic section is present, its exe bit tells us whether or not the object
+        // file requires an executable stack.
+        if sec.name().unwrap_or_default() == GNU_STACK {
+            assert!(gnu_stack_exe.is_none(), "multiple {GNU_STACK} sections");
+            if is_exe {
+                gnu_stack_exe = Some(ExeStack::ExeGnuStackSection);
+            } else {
+                gnu_stack_exe = Some(ExeStack::None);
+            }
+        }
+
+        // Otherwise, just keep track of whether or not we have exeuctable sections
+        has_exe_sections |= is_exe;
+    }
+
+    if let Some(exe) = gnu_stack_exe {
+        return exe;
+    }
+
+    // Ignore object files that have no executable sections, like rmeta
+    if !has_exe_sections {
+        return ExeStack::None;
+    }
+
+    if platform_default_exe_stack_required(obj.architecture()) {
+        ExeStack::NoGnuStackSection
+    } else {
+        ExeStack::None
+    }
+}
+
+/// Default if there is no `.note.GNU-stack` section.
+fn platform_default_exe_stack_required(arch: Architecture) -> bool {
+    match arch {
+        // PPC64 doesn't set `.note.GNU-stack` since GNU nested functions don't need a trampoline,
+        // <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21098>.
+        Architecture::PowerPc64 => false,
+        _ => true,
+    }
+}
+
 /// Thin wrapper for owning data used by `object`.
 struct BinFile {
     path: PathBuf,
@@ -380,6 +494,7 @@ impl BinFile {
     /// D something with each symbol in an archive or object file.
     fn for_each_symbol(&self, mut f: impl FnMut(Symbol, &ObjFile, &str)) {
         self.for_each_object(|obj, obj_path| {
+            dbg!(&obj);
             obj.symbols().for_each(|sym| f(sym, &obj, obj_path));
         });
     }

crates/symbol-check/tests/all.rs

@@ -1,8 +1,8 @@
-use std::env;
 use std::ffi::OsString;
 use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
 use std::sync::LazyLock;
+use std::{env, fs};
 
 use assert_cmd::assert::Assert;
 use assert_cmd::cargo::cargo_bin_cmd;
@@ -72,6 +72,75 @@ fn test_core_symbols() {
         .stderr_contains("from_utf8");
 }
 
+/// Check with an object that has no `.note.GNU-stack` section, indicating platform-default stack
+/// writeability (usually enabled).
+#[test]
+fn test_missing_gnu_stack() {
+    let target = target();
+    let dir = tempdir().unwrap();
+    let mut src = input_dir().join("missing_gnu_stack_section.S");
+    if target.contains("-windows-msvc") {
+        // cc needs the .asm extension to build assembly source on Windows
+        let to = src.join(src.file_stem().unwrap()).with_extension(".asm");
+        fs::copy(&src, &to).unwrap();
+        src = to;
+    }
+
+    let objs = cc_build().file(src).out_dir(&dir).compile_intermediates();
+    let [obj] = objs.as_slice() else {
+        panic!(">1 output")
+    };
+
+    let assert = cargo_bin_cmd!().arg("--check").arg(obj).assert();
+
+    if target.contains("-windows-msvc")
+        || target.contains("-apple-")
+        || target.starts_with("powerpc64")
+    {
+        // Non-ELF targets don't have executable stacks marked in the same way, and ppc64 doesn't
+        // emit `.note.GNU-stack
+        assert.success();
+        return;
+    }
+
+    assert
+        .failure()
+        .stderr_contains("the following object files require an executable stack")
+        .stderr_contains("missing_gnu_stack_section.o");
+}
+
+/// Check with an object that has a `.note.GNU-stack` section with the executable flag set.
+#[test]
+fn check_explicit_exe_gnu_stack() {
+    let mut build = cc_build();
+    if !build.get_compiler().is_like_gnu() {
+        eprintln!("Can't run execstack test; non-GNU compiler");
+        return;
+    }
+
+    let dir = tempdir().unwrap();
+    let objs = build
+        .file(input_dir().join("has_exe_gnu_stack_section.c"))
+        .out_dir(&dir)
+        .compile_intermediates();
+    let [obj] = objs.as_slice() else {
+        panic!(">1 output")
+    };
+
+    let assert = cargo_bin_cmd!().arg("--check").arg(obj).assert();
+
+    if target().starts_with("powerpc64") {
+        // Ppc64 doesn't emit `.note.GNU-stack`
+        assert.success();
+        return;
+    }
+
+    assert
+        .failure()
+        .stderr_contains("the following object files require an executable stack")
+        .stderr_contains("has_exe_gnu_stack_section.o");
+}
+
 #[test]
 fn test_good() {
     let dir = tempdir().unwrap();
@@ -98,7 +167,11 @@ fn rustc_build(i: &Path, o: &Path, mut f: impl FnMut(&mut Command) -> &mut Comma
 /// Configure `cc` with the host and target.
 fn cc_build() -> cc::Build {
     let mut b = cc::Build::new();
-    b.host(env!("HOST")).target(&target());
+    b.host(env!("HOST"))
+        .target(&target())
+        .opt_level(0)
+        .cargo_debug(true)
+        .cargo_metadata(false);
     b
 }
 

crates/symbol-check/tests/input/has_exe_gnu_stack_section.c

@@ -0,0 +1,16 @@
+/* A file that requires an executable stack and thus will have a
+ * `.note.GNU-stack` section with the executable bit set.
+ *
+ * GNU nested functions are the only way I could find to force an explicitly
+ * executable stack. Supported by GCC only, not Clang.
+ */
+
+void intermediate(void (*)(int, int), int);
+
+void hack(int *array, int size) {
+    void store (int index, int value) {
+        array[index] = value;
+    }
+
+    intermediate(store, size);
+}

crates/symbol-check/tests/input/missing_gnu_stack_section.S

@@ -0,0 +1,19 @@
+/* Create an object file with no `.note.GNU-stack` section.
+ *
+ * Assembly files do not get that section, meaning platform-default stack
+ * executability is implied (usually yes on Linux).
+ */
+
+.global func
+
+#ifdef __wasm__
+.functype func () -> ()
+.type     func, @function
+#endif
+
+func:
+    nop
+
+#ifdef __wasm__
+    end_function
+#endif