GrapheneOS / linux-hardened

Minimal supplement to upstream Kernel Self Protection Project changes. Features already provided by SELinux + Yama and archs other than multiarch arm64 / x86_64 aren't in scope. Only tags have stable history. Shared IRC channel with KSPP: irc.freenode.net ##linux-hardened. Currently maintained at https://github.com/anthraxx/linux-hardened.
https://grapheneos.org/
Other
390 stars 102 forks source link

missing compiler check for "-fsanitize=local-init" support #73

Closed alexw65500 closed 6 years ago

alexw65500 commented 6 years ago

When you enable CONFIG_LOCAL_INIT in the kernel you can get a wrong error message, telling the user that something is not supported by the compiler which is in fact not the problem at all.

I was finally able to trace that down for me to the not supported option "-fsanitize=local-init. It looks like https://github.com/copperhead/linux-hardened/commit/487d0122af4ff879889250c981348bb0b48d4864 should also have added a test if the compiler supports -fsanitize=local-init. Without that the first flag being tested is reported as not working.

Here how it looked for me:

  CHK     include/config/kernel.release
  HOSTCC  scripts/basic/bin2c
  UPD     include/config/kernel.release
Cannot use CONFIG_CC_STACKPROTECTOR_REGULAR: -fstack-protector not supported by compiler
make: *** [Makefile:1097: prepare-compiler-check] Error 1
make: *** Waiting for unfinished jobs....

Disabling CONFIG_LOCAL_INIT fixed the kernel build.

thestinger commented 6 years ago

You're not using 4.15.2.a, that's not the error that will be shown.

thestinger commented 6 years ago

Would using $(call cc-option,-fsanitize=local-init) even warn? This use case needs it to error out, and I don't think it's worth writing something similar to the -fstack-protector test. I already moved it so that it doesn't cause that test to fail.

alexw65500 commented 6 years ago

You are right, I'm using the current git version (ok, may be now some hours old) and not the release. I'll make sure to cross check issues with the official releases next time, sorry for the bother.

thestinger commented 6 years ago

The current Git version as in the 4.15 branch? It looks like you're using 4.14.

alexw65500 commented 6 years ago

yes, I'm on the 4.15 branch and also got a vmlinuz-4.15.2 as kernel.

I still have it in the scroll back buffer:

owl /usr/src/linux-hardened # git fetch --all
Fetching origin
remote: Counting objects: 3755, done.
remote: Compressing objects: 100% (330/330), done.
remote: Total 3755 (delta 3097), reused 3208 (delta 2992), pack-reused 433
Receiving objects: 100% (3755/3755), 686.74 KiB | 48.00 KiB/s, done.
Resolving deltas: 100% (3186/3186), completed with 730 local objects.
From https://github.com/copperhead/linux-hardened
 + 54c08027ad47...ef0ac5342827 4.14       -> origin/4.14  (forced update)
 + ff98adaad10d...487d0122af4f 4.15       -> origin/4.15  (forced update)
 * [new tag]                   4.14.18.a  -> 4.14.18.a
 * [new tag]                   4.15.2.a   -> 4.15.2.a
owl /usr/src/linux-hardened # git reset --hard origin/4.15
HEAD is now at 487d0122af4f wire up -fsanitize=local-init
thestinger commented 6 years ago

Ah, I see moving it didn't help. Well, if you're interested in making it friendlier while still having a hard error, I'll accept an uninvasive patch for that.

thestinger commented 6 years ago

I moved it behind the clang check so I don't see to hear about it as often but it doesn't check for the patched Clang toolchain and I won't spend time on that myself.

alexw65500 commented 6 years ago

I tried my hand on a proper fix, but not sure if that still qualify as not invasive.

Problem is, the existing checks for STACKPROTECTOR are already broken - at least for my level of understanding. They omit to first remove the to-be-tested option from CC_OPTION_CFLAGS, making it irrelevant which option you then actually test. They either all work or not. That kind of works as long as we only the test for STACKPROTECTOR, but makes it impossible to check separate flags one by one.

The patch below therefore attempts to first fix the STACKPROTECTOR checks and then leveraging the existing mechanism for -fsanitize=local-sanitize.

It all drills down if it's ok to append to GCC_PLUGINS_CFLAGS as I'm doing it here.

commit 620b25553b584d9ee6ae7997342a0dbad6705e2d
Author: Alexander Wetzel <alexander.wetzel@web.de>
Date:   Sat Feb 10 12:43:22 2018 +0100

    check for "-fsanitize=local-sanitize" support

    Output correct error message if compiler does not support
    "-fsanitize=local-sanitize".

    Also fixes the existing CONFIG_CC_STACKPROTECTOR compiler checks,
    allowing us to probe different flags independently.

diff --git a/Makefile b/Makefile
index f6080da99e5f..8f79e10f1684 100644
--- a/Makefile
+++ b/Makefile
@@ -695,11 +695,14 @@ ifdef CONFIG_CC_STACKPROTECTOR
   stackp-check := $(wildcard $(stackp-path))
 endif
 KBUILD_CFLAGS += $(stackp-flag)
+GCC_PLUGINS_CFLAGS += $(stackp-flag)

-ifeq ($(cc-name),clang)
 ifdef CONFIG_LOCAL_SANITIZE
 KBUILD_CFLAGS   += -fsanitize=local-sanitize
+GCC_PLUGINS_CFLAGS += -fsanitize=local-sanitize
 endif
+
+ifeq ($(cc-name),clang)
 KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
 KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
 KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
@@ -1104,6 +1107,13 @@ ifdef stackp-check
                   $(stackp-flag) available but compiler is broken >&2 && exit 1
   endif
 endif
+# Check if our compiler supports -fsanitize=local-sanitize
+ifdef CONFIG_LOCAL_SANITIZE
+  ifeq ($(call cc-option,-fsanitize=local-sanitize),)
+   @echo Cannot enable CONFIG_LOCAL_SANITIZE: \
+         -fsanitize=local-sanitize not supported by compiler >&2 && exit 1
+  endif
+endif
    @:

 # Generate some files

Edit: updated patch

thestinger commented 6 years ago

Why does adding to GCC_PLUGINS_CFLAGS help here?

alexw65500 commented 6 years ago

Here the interesting code from Kbuild.include:

# __cc-option
# Usage: MY_CFLAGS += $(call __cc-option,$(CC),$(MY_CFLAGS),-march=winchip-c6,-march=i586)
__cc-option = $(call try-run-cached,\
        $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))

# Do not attempt to build with gcc plugins during cc-option tests.
# (And this uses delayed resolution so the flags will be up to date.)
CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))

# cc-option
# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)

cc-option = $(call __cc-option, $(CC),\
        $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS),$(1),$(2))

The function cc-option takes KBUILD_CPPFLAGS, appends all options from CC_OPTION_CFLAGS and calls __cc-option with the merged string as second and the option we test for as third argument. Since KBUILD_CPPFLAGS != KBUILD_CFLAGS we can ignore the KBUILD_CPPFLAGS here, it's just distracting us with a similar name.

The interesting variable is CC_OPTION_CFLAGS, which is KBUILD_CFLAGS minus all strings from GCC_PLUGINS_CFLAGS.

By adding $(stackp-flag) and -fsanitize=local-sanitize (if set) to GCC_PLUGINS_CFLAGS we strip all "tricky" options we want to check for from the default calls of cc-option, allowing us to test them one by one at our leisure, restoring the test as it was (probably) intended by upstream.

Or the other way round: Calling cc-option to test any option set in KBUILD_CFLAGS but not in GCC_PLUGINS_CFLAGS is mostly pointless. cc-option will only check all options set in KBUILD_CFLAGS at once, not allowing us to find out which option failed. Of course it kind of works in upstream with only one option to test for... but I can't image that it was intended in that way.

thestinger commented 6 years ago

I don't consider this a linux-hardened issue at the moment. We'll wait for better infrastructure for adding flags like this.