Skip to content
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

Add windows-kernel flag #39

Closed
wants to merge 1 commit into from
Closed

Add windows-kernel flag #39

wants to merge 1 commit into from

Conversation

carlos-al
Copy link

To resolve Issue #38

@athre0z
Copy link
Member

athre0z commented Jun 9, 2024

Hmmm. As demonstrated by the CI, the drawback of making this a feature flag is that building with --all-features will no longer work. Perhaps instead of linking the overflow library, we can instruct the C compiler to not build with stack protectors. That'd not break anything even if it is enabled in other builds and should solve the issue as well.

Out of curiosity: what target tuple are you using for the Windows kernel builds?

For MSVC it should probably be this flag: /GS-. For GCC/LLVM, it'd be -fno-stack-protector.

@carlos-al
Copy link
Author

Indeed it breaks them, excuse not noting this before.

My target is x86_64-pc-windows-msvc, I'm building with #https://github.com/carlos-al/windows-kernel-rs. This fix could be moved to windows-kernel-sys/build.rs too, and it will work the same. KO when the line is not present, OK when it is.

As for the /GS- flag, I've tried setting it but unfortunately wouldn't make a difference. I've been trying to do it this way for a couple hours to no avail. Modifying both .cargo/config file to include "-C", "link-arg=/GS-" (also as pre-link-arg in case), printing it in build.rs as println!("cargo:rustc-link-arg=/GS-"); both within windows-kernel-sys/build.rs and this project's, but still KO, same imports as noted on the issue. Have retested all these combinations just now after your comment.

Feel free to propose any other modifications and I'll test them.

Note that there are a couple extra ntoskrnl.exe imports too when kernel32.dll is imported.

@carlos-al
Copy link
Author

passing it directly to rustc is not recognised, neither as a -C parameter nor -Z one, and this should be a rustc parameter only as per the VS solution. Changing the flag under "code generation" on VS to be /GS- and removing the reference to BufferOverflowK works too. Doesn't seem rustc supports this when using msvc, just with LLVM rust-lang/rust#84197

@athre0z
Copy link
Member

athre0z commented Jun 9, 2024

I think the issue is that this is a codegen argument, not a linker flag. It's the codegen / compiler that emits the stack cookie checks. Further, we want to pass it to the C compiler, not rustc. Can you see whether the following works:

diff --git a/Cargo.toml b/Cargo.toml
index 6084447..9abec25 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -30,6 +30,7 @@ formatter = ["alloc", "full-decoder"]
 encoder = ["alloc", "full-decoder"]
 serialization = ["serde", "bitflags/serde"]
 nolibc = []
+no-stack-protector = []
 
 [[example]]
 name = "pattern"
diff --git a/build.rs b/build.rs
index 707439c..95b4470 100644
--- a/build.rs
+++ b/build.rs
@@ -35,11 +35,18 @@ fn build_library() {
         bool2cmake(env::var("CARGO_FEATURE_NOLIBC").is_ok()),
     );
 
-    let dst = config.build();
-
     let target = env::var("TARGET").unwrap_or("(unknown)".to_string());
     let is_msvc = target.ends_with("windows-msvc");
 
+    if env::var("CARGO_FEATURE_NO_STACK_PROTECTOR").is_ok() {
+        if is_msvc {
+            config.cflag("/GS-");
+        } else {
+            config.cflag("-fno-stack-protector");
+        }
+    }
+
+    let dst = config.build();
     let relative_build_dir = if is_msvc { config.get_profile() } else { "" };
 
     println!(

I also pushed this to branch no-stack-prot.

@carlos-al
Copy link
Author

It does work as intended. Thanks for the prompt action.

@athre0z
Copy link
Member

athre0z commented Jun 9, 2024

Cool, thanks for reporting and testing this! :) I merged the branch into master, so let's close this here.

@athre0z athre0z closed this Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants