lib/crypto: blake2s: fix a CFI failure

typedef void (*blake2s_compress_t)(struct blake2s_state *state,

				   const u8 *block, size_t nblocks, u32 inc);

should become

typedef void (*blake2s_compress_t)(struct blake2s_state *state,

				   const u8 *block, size_t nblocks, const u32 inc);

Does making that change fix things for you?

Thanks,
Jason

Hi Miles,

Thanks for the patch. Could you let me know which architecture and compiler this was broken on? If I had to guess, I'd wager arm32, and you hit this by enabling optimized blake2s?

If so, I'm not sure the problem is with weak symbols. Why should CFI break weak symbols? Rather, perhaps the issue is that the function is defined in blake2s-core.S? Are there some CFI macros we need for that definition?

(+ Sami, Eric)

Quoted 12 lines · Jason A. Donenfeld

Hi Miles,

Thanks for the patch. Could you let me know which architecture and compiler this was broken on? If I had to guess, I'd wager arm32, and you hit this by enabling optimized blake2s?

If so, I'm not sure the problem is with weak symbols. Why should CFI break weak symbols? Rather, perhaps the issue is that the function is defined in blake2s-core.S? Are there some CFI macros we need for that definition?

We should try to understand why CFI thinks the prototypes of the two symbols are different. There are still a number of issues with CFI, so papering over them by reverting stuff that we want for good reasons is not the way to go imo.

In the short term, you can work around it by avoiding the indirect call to blake2s_compress, e.g.,

lib/crypto/blake2s.c +2 -0

index 93f2ae051370..fef2ff678431 100644
@@ -16,9 +16,15 @@
 #include <linux/init.h>
 #include <linux/bug.h>
 
+static void __blake2s_compress(struct blake2s_state *state, const u8 *block,
+                              size_t nblocks, const u32 inc)

Ehm, maybe not. As Jason points out, the typedef does not have quite the right type, so that is most likely the culprit, and this workaround would trigger CFI in exactly the same way.

Interestingly, the compiler does not seem to mind, right? Or are you seeing any build time warnings on the reference to blake2s_compress in the call to __blake2s_update() ?

We should try to understand why CFI thinks the prototypes of the two symbols are different. There are still a number of issues with CFI, so papering over them by reverting stuff that we want for good reasons is not the way to go imo.

In the short term, you can work around it by avoiding the indirect call to blake2s_compress, e.g.,

Thanks for the patch. Could you let me know which architecture and compiler this was broken on? If I had to guess, I'd wager arm32, and you hit this by enabling optimized blake2s?

If so, I'm not sure the problem is with weak symbols. Why should CFI break weak symbols? Rather, perhaps the issue is that the function is defined in blake2s-core.S? Are there some CFI macros we need for that definition?

Hi Miles,

I'm actually not able to reproduce your oops. I'm using vanilla clang 13, cross compiling for arm64, with thin LTO enabled and CFI enabled. Kernel seems to run fine.

Are there other settings that are needed to trigger this? Do you see it in upstream clang or just the Android fork of clang?

Hi Miles,

Okay. Keep me posted.

Just FYI, as mentioned, I'm unable to reproduce this, and you haven't provided any further minimized guidance on how I might reproduce this, so it'll sit in the "not a bug" bin until I have another clue on how to reproduce. Alternatively, Nick and Nathan are now on this thread and they usually have good luck teasing out compiler issues and such, so maybe they'll have an idea. But I'm afraid with the information I currently have, I'm at a dead end.

Jason

The below kludge of a patch fixes the issue. Still unclear whether we should go with something like this or get clang fixed or what.

arch/arm/crypto/blake2s-shash.c +2 -2

index 17c1c3bfe2f5..be8cde5f1719 100644
@@ -13,12 +13,12 @@
 static int crypto_blake2s_update_arm(struct shash_desc *desc,
                                     const u8 *in, unsigned int inlen)
 {
-       return crypto_blake2s_update(desc, in, inlen, blake2s_compress);
+       return crypto_blake2s_update(desc, in, inlen);
 }
 
 static int crypto_blake2s_final_arm(struct shash_desc *desc, u8 *out)
 {
-       return crypto_blake2s_final(desc, out, blake2s_compress);
+       return crypto_blake2s_final(desc, out);
 }
 
 #define BLAKE2S_ALG(name, driver_name, digest_size)                    \

arch/x86/crypto/blake2s-shash.c +2 -2

index f9e2fecdb761..c81ffedb4865 100644
@@ -18,12 +18,12 @@
 static int crypto_blake2s_update_x86(struct shash_desc *desc,
                                     const u8 *in, unsigned int inlen)
 {
-       return crypto_blake2s_update(desc, in, inlen, blake2s_compress);
+       return crypto_blake2s_update(desc, in, inlen);
 }
 
 static int crypto_blake2s_final_x86(struct shash_desc *desc, u8 *out)
 {
-       return crypto_blake2s_final(desc, out, blake2s_compress);
+       return crypto_blake2s_final(desc, out);
 }
 
 #define BLAKE2S_ALG(name, driver_name, digest_size)                    \

crypto/blake2s_generic.c +0 -0

index 72fe480f9bd6..050874588a84 100644
@@ -5,6 +5,7 @@
  * Copyright (C) 2015-2019 Jason A. Donenfeld <[email protected]>. All

Rights Reserved.
*/

+#define FORCE_BLAKE2S_GENERIC
#include <crypto/internal/blake2s.h>
#include <crypto/internal/hash.h>

I'd prefer it if we could avoid magic #define's like this.

blake2s_compress_generic is weakly aliased to blake2s_generic. The

current harness for function selection uses a function pointer, which is ordinarily inlined and resolved at compile time. But when Clang's CFI is enabled, CFI still triggers when making an indirect call via a weak symbol. This seems like a bug in Clang's CFI, as though it's bucketing weak symbols and strong symbols differently. It also only seems to trigger when "full LTO" mode is used, rather than "thin LTO".

[    0.000000][    T0] Kernel panic - not syncing: CFI failure (target: blake2s_compress_generic+0x0/0x1444)
[    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.16.0-mainline-06981-g076c855b846e #1
[    0.000000][    T0] Hardware name: MT6873 (DT)
[    0.000000][    T0] Call trace:
[    0.000000][    T0]  dump_backtrace+0xfc/0x1dc
[    0.000000][    T0]  dump_stack_lvl+0xa8/0x11c
[    0.000000][    T0]  panic+0x194/0x464
[    0.000000][    T0]  __cfi_check_fail+0x54/0x58
[    0.000000][    T0]  __cfi_slowpath_diag+0x354/0x4b0
[    0.000000][    T0]  blake2s_update+0x14c/0x178
[    0.000000][    T0]  _extract_entropy+0xf4/0x29c
[    0.000000][    T0]  crng_initialize_primary+0x24/0x94
[    0.000000][    T0]  rand_initialize+0x2c/0x6c
[    0.000000][    T0]  start_kernel+0x2f8/0x65c
[    0.000000][    T0]  __primary_switched+0xc4/0x7be4
[    0.000000][    T0] Rebooting in 5 seconds..

Nonetheless, the function pointer method isn't so terrific anyway, so this patch replaces it with a simple boolean, which also gets inlined away. This successfully works around the Clang bug.

In general, I'm not too keen on all of the indirection involved here; it clearly does more harm than good. Hopefully the whole thing can get cleaned up down the road when lib/crypto is overhauled more comprehensively. But for now, we go with a simple bandaid.

Fixes 6048fdcc5f26 ("lib/crypto: blake2s: include as built-in")

Reported-by Miles Chen <[email protected]>

Cc Nick Desaulniers <[email protected]>

Cc Sami Tolvanen <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Signed-off-by Jason A. Donenfeld <[email protected]>

---
 arch/arm/crypto/blake2s-shash.c   |  4 ++--
 arch/x86/crypto/blake2s-shash.c   |  4 ++--
 crypto/blake2s_generic.c          |  4 ++--
 include/crypto/internal/blake2s.h | 36 ++++++++++++++++++-------------
 lib/crypto/blake2s.c              |  4 ++--
 5 files changed, 29 insertions(+), 23 deletions(-)

arch/arm/crypto/blake2s-shash.c +2 -2

index 17c1c3bfe2f5..763c73beea2d 100644
@@ -13,12 +13,12 @@
 static int crypto_blake2s_update_arm(struct shash_desc *desc,
 				     const u8 *in, unsigned int inlen)
 {
-	return crypto_blake2s_update(desc, in, inlen, blake2s_compress);
+	return crypto_blake2s_update(desc, in, inlen, false);
 }
 
 static int crypto_blake2s_final_arm(struct shash_desc *desc, u8 *out)
 {
-	return crypto_blake2s_final(desc, out, blake2s_compress);
+	return crypto_blake2s_final(desc, out, false);
 }
 
 #define BLAKE2S_ALG(name, driver_name, digest_size)			\

arch/x86/crypto/blake2s-shash.c +2 -2

index f9e2fecdb761..59ae28abe35c 100644
@@ -18,12 +18,12 @@
 static int crypto_blake2s_update_x86(struct shash_desc *desc,
 				     const u8 *in, unsigned int inlen)
 {
-	return crypto_blake2s_update(desc, in, inlen, blake2s_compress);
+	return crypto_blake2s_update(desc, in, inlen, false);
 }
 
 static int crypto_blake2s_final_x86(struct shash_desc *desc, u8 *out)
 {
-	return crypto_blake2s_final(desc, out, blake2s_compress);
+	return crypto_blake2s_final(desc, out, false);
 }
 
 #define BLAKE2S_ALG(name, driver_name, digest_size)			\

crypto/blake2s_generic.c +2 -2

index 72fe480f9bd6..5f96a21f8788 100644
@@ -15,12 +15,12 @@
 static int crypto_blake2s_update_generic(struct shash_desc *desc,
 					 const u8 *in, unsigned int inlen)
 {
-	return crypto_blake2s_update(desc, in, inlen, blake2s_compress_generic);
+	return crypto_blake2s_update(desc, in, inlen, true);
 }
 
 static int crypto_blake2s_final_generic(struct shash_desc *desc, u8 *out)
 {
-	return crypto_blake2s_final(desc, out, blake2s_compress_generic);
+	return crypto_blake2s_final(desc, out, true);
 }
 
 #define BLAKE2S_ALG(name, driver_name, digest_size)			\

include/crypto/internal/blake2s.h +21 -15

index d39cfa0d333e..e2da9b413d44 100644
@@ -24,14 +24,11 @@ static inline void blake2s_set_lastblock(struct blake2s_state *state)
 	state->f[0] = -1;
 }
 
-typedef void (*blake2s_compress_t)(struct blake2s_state *state,
-				   const u8 *block, size_t nblocks, u32 inc);
-
 /* Helper functions for BLAKE2s shared by the library and shash APIs */
 
-static inline void __blake2s_update(struct blake2s_state *state,
-				    const u8 *in, size_t inlen,
-				    blake2s_compress_t compress)
+static __always_inline void __blake2s_update(struct blake2s_state *state,
+					     const u8 *in, size_t inlen,
+					     bool force_generic)
 {
 	const size_t fill = BLAKE2S_BLOCK_SIZE - state->buflen;
 

@@ -39,7 +36,10 @@ static inline void __blake2s_update(struct blake2s_state *state,
 		return;
 	if (inlen > fill) {
 		memcpy(state->buf + state->buflen, in, fill);
-		(*compress)(state, state->buf, 1, BLAKE2S_BLOCK_SIZE);
+		if (force_generic)
+			blake2s_compress_generic(state, state->buf, 1, BLAKE2S_BLOCK_SIZE);
+		else
+			blake2s_compress(state, state->buf, 1, BLAKE2S_BLOCK_SIZE);
 		state->buflen = 0;
 		in += fill;
 		inlen -= fill;

@@ -47,7 +47,10 @@ static inline void __blake2s_update(struct blake2s_state *state,
 	if (inlen > BLAKE2S_BLOCK_SIZE) {
 		const size_t nblocks = DIV_ROUND_UP(inlen, BLAKE2S_BLOCK_SIZE);
 		/* Hash one less (full) block than strictly possible */
-		(*compress)(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
+		if (force_generic)
+			blake2s_compress_generic(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
+		else
+			blake2s_compress(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
 		in += BLAKE2S_BLOCK_SIZE * (nblocks - 1);
 		inlen -= BLAKE2S_BLOCK_SIZE * (nblocks - 1);
 	}

@@ -55,13 +58,16 @@ static inline void __blake2s_update(struct blake2s_state *state,
 	state->buflen += inlen;
 }
 
-static inline void __blake2s_final(struct blake2s_state *state, u8 *out,
-				   blake2s_compress_t compress)
+static __always_inline void __blake2s_final(struct blake2s_state *state, u8 *out,
+					    bool force_generic)
 {
 	blake2s_set_lastblock(state);
 	memset(state->buf + state->buflen, 0,
 	       BLAKE2S_BLOCK_SIZE - state->buflen); /* Padding */
-	(*compress)(state, state->buf, 1, state->buflen);
+	if (force_generic)
+		blake2s_compress_generic(state, state->buf, 1, state->buflen);
+	else
+		blake2s_compress(state, state->buf, 1, state->buflen);
 	cpu_to_le32_array(state->h, ARRAY_SIZE(state->h));
 	memcpy(out, state->h, state->outlen);
 }

@@ -99,20 +105,20 @@ static inline int crypto_blake2s_init(struct shash_desc *desc)
 
 static inline int crypto_blake2s_update(struct shash_desc *desc,
 					const u8 *in, unsigned int inlen,
-					blake2s_compress_t compress)
+					bool force_generic)
 {
 	struct blake2s_state *state = shash_desc_ctx(desc);
 
-	__blake2s_update(state, in, inlen, compress);
+	__blake2s_update(state, in, inlen, force_generic);
 	return 0;
 }
 
 static inline int crypto_blake2s_final(struct shash_desc *desc, u8 *out,
-				       blake2s_compress_t compress)
+				       bool force_generic)
 {
 	struct blake2s_state *state = shash_desc_ctx(desc);
 
-	__blake2s_final(state, out, compress);
+	__blake2s_final(state, out, force_generic);
 	return 0;
 }
 

lib/crypto/blake2s.c +2 -2

index 9364f79937b8..c71c09621c09 100644
@@ -18,14 +18,14 @@
 
 void blake2s_update(struct blake2s_state *state, const u8 *in, size_t inlen)
 {
-	__blake2s_update(state, in, inlen, blake2s_compress);
+	__blake2s_update(state, in, inlen, false);
 }
 EXPORT_SYMBOL(blake2s_update);
 
 void blake2s_final(struct blake2s_state *state, u8 *out)
 {
 	WARN_ON(IS_ENABLED(DEBUG) && !out);
-	__blake2s_final(state, out, blake2s_compress);
+	__blake2s_final(state, out, false);
 	memzero_explicit(state, sizeof(*state));
 }
 EXPORT_SYMBOL(blake2s_final);

Signature · 1 line

2.34.1

Quoted 1 line · Jason A. Donenfeld

blake2s_compress_generic is weakly aliased to blake2s_generic. The

Isn't it the other way around?

blake2s_compress_generic is weakly aliased to blake2s_generic. The current harness for function selection uses a function pointer, which is ordinarily inlined and resolved at compile time. But when Clang's CFI is enabled, CFI still triggers when making an indirect call via a weak symbol. This seems like a bug in Clang's CFI, as though it's bucketing weak symbols and strong symbols differently. It also only seems to trigger when "full LTO" mode is used, rather than "thin LTO".

[    0.000000][    T0] Kernel panic - not syncing: CFI failure (target: blake2s_compress_generic+0x0/0x1444)
[    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.16.0-mainline-06981-g076c855b846e #1
[    0.000000][    T0] Hardware name: MT6873 (DT)
[    0.000000][    T0] Call trace:
[    0.000000][    T0]  dump_backtrace+0xfc/0x1dc
[    0.000000][    T0]  dump_stack_lvl+0xa8/0x11c
[    0.000000][    T0]  panic+0x194/0x464
[    0.000000][    T0]  __cfi_check_fail+0x54/0x58
[    0.000000][    T0]  __cfi_slowpath_diag+0x354/0x4b0
[    0.000000][    T0]  blake2s_update+0x14c/0x178
[    0.000000][    T0]  _extract_entropy+0xf4/0x29c
[    0.000000][    T0]  crng_initialize_primary+0x24/0x94
[    0.000000][    T0]  rand_initialize+0x2c/0x6c
[    0.000000][    T0]  start_kernel+0x2f8/0x65c
[    0.000000][    T0]  __primary_switched+0xc4/0x7be4
[    0.000000][    T0] Rebooting in 5 seconds..

Nonetheless, the function pointer method isn't so terrific anyway, so this patch replaces it with a simple boolean, which also gets inlined away. This successfully works around the Clang bug.

In general, I'm not too keen on all of the indirection involved here; it clearly does more harm than good. Hopefully the whole thing can get cleaned up down the road when lib/crypto is overhauled more comprehensively. But for now, we go with a simple bandaid.

Fixes 6048fdcc5f26 ("lib/crypto: blake2s: include as built-in")

Reported-by Miles Chen <[email protected]>

Cc Nick Desaulniers <[email protected]>

Cc Sami Tolvanen <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Signed-off-by Jason A. Donenfeld <[email protected]>

---
 arch/arm/crypto/blake2s-shash.c   |  4 ++--
 arch/x86/crypto/blake2s-shash.c   |  4 ++--
 crypto/blake2s_generic.c          |  4 ++--
 include/crypto/internal/blake2s.h | 36 ++++++++++++++++++-------------
 lib/crypto/blake2s.c              |  4 ++--
 5 files changed, 29 insertions(+), 23 deletions(-)

There are some lines over 80 columns in this patch.

Otherwise this looks fine. It would be really nice to fix this in clang, though.

Quoted 1 line · Eric Biggers

There are some lines over 80 columns in this patch.

I'll fix that up.

Quoted 2 lines

Otherwise this looks fine. It would be really nice to fix this in clang, though.

I agree. I'm wondering whether it makes sense to roll with this patch, or to just not change anything and lean on Clang upstream to fix their bug. Full LTO is marked as "experimental" still. On the other hand, Android ships with it turned on, so how "experimental" can it really be? I don't have a strong read on the Clang ecosystem to know what makes most sense, to apply this or not. Do you have an opinion?

blake2s_compress_generic is weakly aliased by blake2s_generic. The current harness for function selection uses a function pointer, which is ordinarily inlined and resolved at compile time. But when Clang's CFI is enabled, CFI still triggers when making an indirect call via a weak symbol. This seems like a bug in Clang's CFI, as though it's bucketing weak symbols and strong symbols differently. It also only seems to trigger when "full LTO" mode is used, rather than "thin LTO".

[    0.000000][    T0] Kernel panic - not syncing: CFI failure (target: blake2s_compress_generic+0x0/0x1444)
[    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.16.0-mainline-06981-g076c855b846e #1
[    0.000000][    T0] Hardware name: MT6873 (DT)
[    0.000000][    T0] Call trace:
[    0.000000][    T0]  dump_backtrace+0xfc/0x1dc
[    0.000000][    T0]  dump_stack_lvl+0xa8/0x11c
[    0.000000][    T0]  panic+0x194/0x464
[    0.000000][    T0]  __cfi_check_fail+0x54/0x58
[    0.000000][    T0]  __cfi_slowpath_diag+0x354/0x4b0
[    0.000000][    T0]  blake2s_update+0x14c/0x178
[    0.000000][    T0]  _extract_entropy+0xf4/0x29c
[    0.000000][    T0]  crng_initialize_primary+0x24/0x94
[    0.000000][    T0]  rand_initialize+0x2c/0x6c
[    0.000000][    T0]  start_kernel+0x2f8/0x65c
[    0.000000][    T0]  __primary_switched+0xc4/0x7be4
[    0.000000][    T0] Rebooting in 5 seconds..

Nonetheless, the function pointer method isn't so terrific anyway, so this patch replaces it with a simple boolean, which also gets inlined away. This successfully works around the Clang bug.

In general, I'm not too keen on all of the indirection involved here; it clearly does more harm than good. Hopefully the whole thing can get cleaned up down the road when lib/crypto is overhauled more comprehensively. But for now, we go with a simple bandaid.

Fixes 6048fdcc5f26 ("lib/crypto: blake2s: include as built-in")

Reported-by Miles Chen <[email protected]>

Tested-by Miles Chen <[email protected]>

Tested-by Nathan Chancellor <[email protected]>

Link https://github.com/ClangBuiltLinux/linux/issues/1567

Cc Nick Desaulniers <[email protected]>

Cc Sami Tolvanen <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Signed-off-by Jason A. Donenfeld <[email protected]>

---
Changes v1->v2:
- Wrapped columns at 80 for Eric.

 arch/arm/crypto/blake2s-shash.c   |  4 ++--
 arch/x86/crypto/blake2s-shash.c   |  4 ++--
 crypto/blake2s_generic.c          |  4 ++--
 include/crypto/internal/blake2s.h | 40 +++++++++++++++++++------------
 lib/crypto/blake2s.c              |  4 ++--
 5 files changed, 33 insertions(+), 23 deletions(-)

arch/arm/crypto/blake2s-shash.c +2 -2

index 17c1c3bfe2f5..763c73beea2d 100644
@@ -13,12 +13,12 @@
 static int crypto_blake2s_update_arm(struct shash_desc *desc,
                                     const u8 *in, unsigned int inlen)
 {
-       return crypto_blake2s_update(desc, in, inlen, blake2s_compress);
+       return crypto_blake2s_update(desc, in, inlen, false);
 }
 
 static int crypto_blake2s_final_arm(struct shash_desc *desc, u8 *out)
 {
-       return crypto_blake2s_final(desc, out, blake2s_compress);
+       return crypto_blake2s_final(desc, out, false);
 }
 
 #define BLAKE2S_ALG(name, driver_name, digest_size)                    \

arch/x86/crypto/blake2s-shash.c +2 -2

index f9e2fecdb761..59ae28abe35c 100644
@@ -18,12 +18,12 @@
 static int crypto_blake2s_update_x86(struct shash_desc *desc,
                                     const u8 *in, unsigned int inlen)
 {
-       return crypto_blake2s_update(desc, in, inlen, blake2s_compress);
+       return crypto_blake2s_update(desc, in, inlen, false);
 }
 
 static int crypto_blake2s_final_x86(struct shash_desc *desc, u8 *out)
 {
-       return crypto_blake2s_final(desc, out, blake2s_compress);
+       return crypto_blake2s_final(desc, out, false);
 }
 
 #define BLAKE2S_ALG(name, driver_name, digest_size)                    \

crypto/blake2s_generic.c +2 -2

index 72fe480f9bd6..5f96a21f8788 100644
@@ -15,12 +15,12 @@
 static int crypto_blake2s_update_generic(struct shash_desc *desc,
                                         const u8 *in, unsigned int inlen)
 {
-       return crypto_blake2s_update(desc, in, inlen, blake2s_compress_generic);
+       return crypto_blake2s_update(desc, in, inlen, true);
 }
 
 static int crypto_blake2s_final_generic(struct shash_desc *desc, u8 *out)
 {
-       return crypto_blake2s_final(desc, out, blake2s_compress_generic);
+       return crypto_blake2s_final(desc, out, true);
 }
 
 #define BLAKE2S_ALG(name, driver_name, digest_size)                    \

include/crypto/internal/blake2s.h +25 -15

index d39cfa0d333e..52363eee2b20 100644
@@ -24,14 +24,11 @@ static inline void blake2s_set_lastblock(struct blake2s_state *state)
        state->f[0] = -1;
 }
 
-typedef void (*blake2s_compress_t)(struct blake2s_state *state,
-                                  const u8 *block, size_t nblocks, u32 inc);
-
 /* Helper functions for BLAKE2s shared by the library and shash APIs */
 
-static inline void __blake2s_update(struct blake2s_state *state,
-                                   const u8 *in, size_t inlen,
-                                   blake2s_compress_t compress)
+static __always_inline void
+__blake2s_update(struct blake2s_state *state, const u8 *in, size_t inlen,
+                bool force_generic)
 {
        const size_t fill = BLAKE2S_BLOCK_SIZE - state->buflen;
 

@@ -39,7 +36,12 @@ static inline void __blake2s_update(struct blake2s_state *state,
                return;
        if (inlen > fill) {
                memcpy(state->buf + state->buflen, in, fill);
-               (*compress)(state, state->buf, 1, BLAKE2S_BLOCK_SIZE);
+               if (force_generic)
+                       blake2s_compress_generic(state, state->buf, 1,
+                                                BLAKE2S_BLOCK_SIZE);
+               else
+                       blake2s_compress(state, state->buf, 1,
+                                        BLAKE2S_BLOCK_SIZE);
                state->buflen = 0;
                in += fill;
                inlen -= fill;

@@ -47,7 +49,12 @@ static inline void __blake2s_update(struct blake2s_state *state,
        if (inlen > BLAKE2S_BLOCK_SIZE) {
                const size_t nblocks = DIV_ROUND_UP(inlen, BLAKE2S_BLOCK_SIZE);
                /* Hash one less (full) block than strictly possible */
-               (*compress)(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
+               if (force_generic)
+                       blake2s_compress_generic(state, in, nblocks - 1,
+                                                BLAKE2S_BLOCK_SIZE);
+               else
+                       blake2s_compress(state, in, nblocks - 1,
+                                        BLAKE2S_BLOCK_SIZE);
                in += BLAKE2S_BLOCK_SIZE * (nblocks - 1);
                inlen -= BLAKE2S_BLOCK_SIZE * (nblocks - 1);
        }

@@ -55,13 +62,16 @@ static inline void __blake2s_update(struct blake2s_state *state,
        state->buflen += inlen;
 }
 
-static inline void __blake2s_final(struct blake2s_state *state, u8 *out,
-                                  blake2s_compress_t compress)
+static __always_inline void
+__blake2s_final(struct blake2s_state *state, u8 *out, bool force_generic)
 {
        blake2s_set_lastblock(state);
        memset(state->buf + state->buflen, 0,
               BLAKE2S_BLOCK_SIZE - state->buflen); /* Padding */
-       (*compress)(state, state->buf, 1, state->buflen);
+       if (force_generic)
+               blake2s_compress_generic(state, state->buf, 1, state->buflen);
+       else
+               blake2s_compress(state, state->buf, 1, state->buflen);
        cpu_to_le32_array(state->h, ARRAY_SIZE(state->h));
        memcpy(out, state->h, state->outlen);
 }

@@ -99,20 +109,20 @@ static inline int crypto_blake2s_init(struct shash_desc *desc)
 
 static inline int crypto_blake2s_update(struct shash_desc *desc,
                                        const u8 *in, unsigned int inlen,
-                                       blake2s_compress_t compress)
+                                       bool force_generic)
 {
        struct blake2s_state *state = shash_desc_ctx(desc);
 
-       __blake2s_update(state, in, inlen, compress);
+       __blake2s_update(state, in, inlen, force_generic);
        return 0;
 }
 
 static inline int crypto_blake2s_final(struct shash_desc *desc, u8 *out,
-                                      blake2s_compress_t compress)
+                                      bool force_generic)
 {
        struct blake2s_state *state = shash_desc_ctx(desc);
 
-       __blake2s_final(state, out, compress);
+       __blake2s_final(state, out, force_generic);
        return 0;
 }
 

lib/crypto/blake2s.c +2 -3

index 9364f79937b8..c71c09621c09 100644
@@ -18,14 +18,14 @@
 
 void blake2s_update(struct blake2s_state *state, const u8 *in, size_t inlen)
 {
-       __blake2s_update(state, in, inlen, blake2s_compress);
+       __blake2s_update(state, in, inlen, false);
 }
 EXPORT_SYMBOL(blake2s_update);
 
 void blake2s_final(struct blake2s_state *state, u8 *out)
 {
        WARN_ON(IS_ENABLED(DEBUG) && !out);
-       __blake2s_final(state, out, blake2s_compress);
+       __blake2s_final(state, out, false);
        memzero_explicit(state, sizeof(*state));
 }
 EXPORT_SYMBOL(blake2s_final);
--

2.34.1

blake2s_compress_generic is weakly aliased by blake2s_generic. The

Fixes 6048fdcc5f26 ("lib/crypto: blake2s: include as built-in")

Reported-by Miles Chen <[email protected]>

Tested-by Miles Chen <[email protected]>

Tested-by Nathan Chancellor <[email protected]>

Link https://github.com/ClangBuiltLinux/linux/issues/1567

Cc Nick Desaulniers <[email protected]>

Cc Sami Tolvanen <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Signed-off-by Jason A. Donenfeld <[email protected]>

---

Changes v1->v2:
- Wrapped columns at 80 for Eric.

Quoted 1 line · Jason A. Donenfeld

blake2s_compress_generic is weakly aliased by blake2s_generic. The

Don't you mean "weakly aliased by blake2s_compress"?

Quoted 2 lines

Changes v1->v2:
- Wrapped columns at 80 for Eric.

It is the recommended coding style, so not just for me :-)

Quoted 4 lines · Eric Biggers

Quoted 1 line · Jason A. Donenfeld

blake2s_compress_generic is weakly aliased by blake2s_generic. The

Don't you mean "weakly aliased by blake2s_compress"?

Grrrr. Thanks.

Quoted 4 lines

Quoted 2 lines

Changes v1->v2:
- Wrapped columns at 80 for Eric.

It is the recommended coding style, so not just for me :-)

I was under the impression this increased to 100 mid-2020 or so, and checkpatch.pl now makes noise at that width instead.

blake2s_compress_generic is weakly aliased to blake2s_generic. The current harness for function selection uses a function pointer, which is ordinarily inlined and resolved at compile time. But when Clang's CFI is enabled, CFI still triggers when making an indirect call via a weak symbol. This seems like a bug in Clang's CFI, as though it's bucketing weak symbols and strong symbols differently. It also only seems to trigger when "full LTO" mode is used, rather than "thin LTO".

[    0.000000][    T0] Kernel panic - not syncing: CFI failure (target: blake2s_compress_generic+0x0/0x1444)
[    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.16.0-mainline-06981-g076c855b846e #1
[    0.000000][    T0] Hardware name: MT6873 (DT)
[    0.000000][    T0] Call trace:
[    0.000000][    T0]  dump_backtrace+0xfc/0x1dc
[    0.000000][    T0]  dump_stack_lvl+0xa8/0x11c
[    0.000000][    T0]  panic+0x194/0x464
[    0.000000][    T0]  __cfi_check_fail+0x54/0x58
[    0.000000][    T0]  __cfi_slowpath_diag+0x354/0x4b0
[    0.000000][    T0]  blake2s_update+0x14c/0x178
[    0.000000][    T0]  _extract_entropy+0xf4/0x29c
[    0.000000][    T0]  crng_initialize_primary+0x24/0x94
[    0.000000][    T0]  rand_initialize+0x2c/0x6c
[    0.000000][    T0]  start_kernel+0x2f8/0x65c
[    0.000000][    T0]  __primary_switched+0xc4/0x7be4
[    0.000000][    T0] Rebooting in 5 seconds..

Sent: 19 January 2022 12:19

From: Ard Biesheuvel

Quoted 1 line

Sent: 19 January 2022 12:19

...

Review

Quoted 6 lines

(quoted diff) +5 -1

-               (*compress)(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
+               if (IS_ENABLED(CONFIG_CRYPTO_ARCH_HAVE_LIB_BLAKE2S))
+                       (*compress)(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
+               else
+                       blake2s_compress_generic(state, in, nblocks - 1,
+                                                BLAKE2S_BLOCK_SIZE);

Isn't that a candidate for a 'static call' ?

And, maybe all these inlined functions should be real functions? No point having all the bloat on every call site.
Much better to call a real function and used the cached instructions.

There are clearly optimisations for the top/bottom of the loop. But they can be done to the generic C version.

We're back in business! I was able to reproduce this using FullLTO rather than ThinLTO.

Hi,

Quoted 7 lines

Hi Miles,

I'm actually not able to reproduce your oops. I'm using vanilla clang 13, cross compiling for arm64, with thin LTO enabled and CFI enabled. Kernel seems to run fine.

Quoted 3 lines

Are there other settings that are needed to trigger this? Do you see it in upstream clang or just the Android fork of clang?

I will try another clang (the previous version I use).
I am using Android fork of clang and there is a clang upgrade in this merge.

Quoted 10 lines · Miles Chen

Hi,

Quoted 7 lines

Hi Miles,

I'm actually not able to reproduce your oops. I'm using vanilla clang 13, cross compiling for arm64, with thin LTO enabled and CFI enabled. Kernel seems to run fine.

Quoted 7 lines

Quoted 3 lines

Are there other settings that are needed to trigger this? Do you see it in upstream clang or just the Android fork of clang?

I will try another clang (the previous version I use).
I am using Android fork of clang and there is a clang upgrade in this merge.

One thing that could be worth a try is to make __blake2s_update() and __blake2s_final() __always_inline rather than just inline, which by itself does not appear to be sufficient for the code to get inlined. (If it were, the indirect call should have disappeared as well)

Given that indirect calls suck on x86, we should probably apply that change in any case, regardless of CFI.

Quoted 19 lines · Ard Biesheuvel

Quoted 10 lines · Miles Chen

Hi,

Quoted 7 lines

Hi Miles,

I'm actually not able to reproduce your oops. I'm using vanilla clang 13, cross compiling for arm64, with thin LTO enabled and CFI enabled. Kernel seems to run fine.

Quoted 7 lines

Quoted 3 lines

Are there other settings that are needed to trigger this? Do you see it in upstream clang or just the Android fork of clang?

I will try another clang (the previous version I use).
I am using Android fork of clang and there is a clang upgrade in this merge.

Quoted 8 lines

One thing that could be worth a try is to make __blake2s_update() and __blake2s_final() __always_inline rather than just inline, which by itself does not appear to be sufficient for the code to get inlined. (If it were, the indirect call should have disappeared as well)

Given that indirect calls suck on x86, we should probably apply that change in any case, regardless of CFI.

Had the same thought at first, but then looking at the original stack trace, it looks like the __ function is inlined:

[    0.000000][    T0]  __cfi_slowpath_diag+0x354/0x4b0
[    0.000000][    T0]  blake2s_update+0x14c/0x178
[    0.000000][    T0]  _extract_entropy+0xf4/0x29c