security: enable software CFI

[enh-google]

until we have https://github.com/google/android-riscv64/issues/15 (which hopefully we'll get in the Android/riscv64 ABI), we should at least enable software-only CFI.

(we had to disable this in the build system's global settings, suggesting at least some llvm work is needed here.)

[enh-google]

@appujee

[appujee]

CFI was disabled in: https://android-review.git.corp.google.com/c/platform/build/soong/+/2263628

[samitolvanen]

Now that #22 has been fixed, is there something else blocking CFI?

[enh-google]

i don't think so. i think we can re-enable scs for riscv64 too now we have the new compiler?

[appujee]

i'm testing scs first, and then i'll try the cfi.

[appujee]

for scs: https://android-review.googlesource.com/c/platform/build/soong/+/2660940

[appujee]

cfi enabled: https://android-review.googlesource.com/c/platform/build/soong/+/2595746

[samitolvanen]

Reopening as CFI was disabled again in https://r.android.com/2684006.

[samitolvanen]

I had some time to look into this issue, and the reason init crashes at boot when CFI is enabled is that the CFI shadow implementation in bionic assumes we have a 48-bit virtual address space, while the Linux kernel happily enables Sv57 for cuttlefish. Increasing kMaxTargetAddr allows cuttlefish to boot again:

diff --git a/libc/private/CFIShadow.h b/libc/private/CFIShadow.h
index cbdf0f706..0bc77350d 100644
--- a/libc/private/CFIShadow.h
+++ b/libc/private/CFIShadow.h
@@ -61,7 +61,9 @@ class CFIShadow {
   // Alignment of __cfi_check.
   static constexpr uintptr_t kCfiCheckAlign = 1UL << kCfiCheckGranularity;  // 4K

-#if defined (__LP64__)
+#if defined(__riscv)
+  static constexpr uintptr_t kMaxTargetAddr = (1UL << 57) - 1;
+#elif defined (__LP64__)
   static constexpr uintptr_t kMaxTargetAddr = 0xffffffffffff;
 #else
   static constexpr uintptr_t kMaxTargetAddr = 0xffffffff;

Of course, I suspect we're not going to need Sv57 any time soon in Android devices, so perhaps we should instead tell the cuttlefish kernel to use Sv48 by adding no5lvl to the kernel command line (or even no4lvl for Sv39). Any thoughts?

[enh-google]

Of course, I suspect we're not going to need Sv57 any time soon in Android devices, so perhaps we should instead tell the cuttlefish kernel to use Sv48 by adding no5lvl to the kernel command line (or even no4lvl for Sv39). Any thoughts?

yeah, if we can make it match arm64, that's always my strong preference. ("one less thing to think about".)

we also have

#elif defined(__riscv)
    // TODO: sv48 and sv57 were both added to the kernel this year, so we
    // probably just need some kernel fixes to enable higher ASLR randomization,
    // but for now 24 is the maximum that the kernel supports.
    if (SetMmapRndBitsMin(24, 18, false)) {
        return {};
    }

in https://cs.android.com/android/platform/superproject/main/+/main:system/core/init/security.cpp;l=119?q=file:core%2Finit%20riscv which it would be nice to be able to fix.

(interestingly i notice that -- because this code predates GKI by a decade -- we have a weaker requirement there for arm64 than probably makes sense any more? do you know if the arm64 GKI is already configured for more? i'd happily bump arm64 to 33/33 :-) )

[samitolvanen]

Of course, I suspect we're not going to need Sv57 any time soon in Android devices, so perhaps we should instead tell the cuttlefish kernel to use Sv48 by adding no5lvl to the kernel command line (or even no4lvl for Sv39). Any thoughts?

yeah, if we can make it match arm64, that's always my strong preference. ("one less thing to think about".)

Yes, that's what I thought too. I'll send a CL.

we also have

#elif defined(__riscv)
    // TODO: sv48 and sv57 were both added to the kernel this year, so we
    // probably just need some kernel fixes to enable higher ASLR randomization,
    // but for now 24 is the maximum that the kernel supports.
    if (SetMmapRndBitsMin(24, 18, false)) {
        return {};
    }

in https://cs.android.com/android/platform/superproject/main/+/main:system/core/init/security.cpp;l=119?q=file:core%2Finit%20riscv which it would be nice to be able to fix.

I plan to look into this next. The kernel still hardcodes the maximum to 24 for RISC-V. I believe the initial devices are going to be Sv39 though, so this is mostly relevant in the long term.

(interestingly i notice that -- because this code predates GKI by a decade -- we have a weaker requirement there for arm64 than probably makes sense any more? do you know if the arm64 GKI is already configured for more? i'd happily bump arm64 to 33/33 :-) )

GKI doesn't seem to change the default value for ARCH_MMAP_RND_BITS_MAX, which has been 33 bits with ARM64_VA_BITS=48 since v4.5:

https://elixir.bootlin.com/linux/v4.5.7/source/arch/arm64/Kconfig#L118

I'm not sure how old kernels we still need to support in the next release, but I suspect bumping the arm64 minimum to 33 should be safe now.

[samitolvanen]

CFI is enabled again: https://r.android.com/q/topic:%22riscv-cfi%22