| Message ID | 20221118090220.398819-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | crypto: CFI fixes | expand |
On Fri, Nov 18, 2022 at 01:02:09AM -0800, Eric Biggers wrote: > This series fixes some crashes when CONFIG_CFI_CLANG (Control Flow > Integrity) is enabled, with the new CFI implementation that was merged > in 6.1 and is supported on x86. Some of them were unconditional > crashes, while others depended on whether the compiler optimized out the > indirect calls or not. This series also simplifies some code that was > intended to work around limitations of the old CFI implementation and is > unnecessary for the new CFI implementation. > > Eric Biggers (11): > crypto: x86/aegis128 - fix crash with CFI enabled > crypto: x86/aria - fix crash with CFI enabled > crypto: x86/nhpoly1305 - eliminate unnecessary CFI wrappers > crypto: x86/sha1 - fix possible crash with CFI enabled > crypto: x86/sha256 - fix possible crash with CFI enabled > crypto: x86/sha512 - fix possible crash with CFI enabled > crypto: x86/sm3 - fix possible crash with CFI enabled > crypto: arm64/nhpoly1305 - eliminate unnecessary CFI wrapper > crypto: arm64/sm3 - fix possible crash with CFI enabled > crypto: arm/nhpoly1305 - eliminate unnecessary CFI wrapper > Revert "crypto: shash - avoid comparing pointers to exported functions > under CFI" These all look good. They will hoever conflict with the alignment cleanups/changes we've got in tip/x86/core, but there's no helping that I support. Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> -----Original Message----- > From: Eric Biggers <ebiggers@kernel.org> > Sent: Friday, November 18, 2022 3:02 AM > To: linux-crypto@vger.kernel.org > Cc: x86@kernel.org; linux-arm-kernel@lists.infradead.org; Sami Tolvanen > <samitolvanen@google.com> > Subject: [PATCH 0/11] crypto: CFI fixes > > This series fixes some crashes when CONFIG_CFI_CLANG (Control Flow > Integrity) is enabled, with the new CFI implementation that was merged > in 6.1 and is supported on x86. Some of them were unconditional > crashes, while others depended on whether the compiler optimized out the > indirect calls or not. This series also simplifies some code that was > intended to work around limitations of the old CFI implementation and is > unnecessary for the new CFI implementation. Some of the x86 modules EXPORT their asm functions. Does that leave them at risk of being called indirectly? arch/x86/crypto/camellia-aesni-avx-asm_64.S:SYM_FUNC_START(camellia_ecb_dec_16way) arch/x86/crypto/camellia-aesni-avx-asm_64.S:SYM_FUNC_START(camellia_ecb_enc_16way) arch/x86/crypto/camellia-aesni-avx-asm_64.S:SYM_FUNC_START(camellia_cbc_dec_16way) arch/x86/crypto/camellia_aesni_avx_glue.c:asmlinkage void camellia_ecb_enc_16way(const void *ctx, u8 *dst, const u8 *src); arch/x86/crypto/camellia_aesni_avx_glue.c:EXPORT_SYMBOL_GPL(camellia_ecb_enc_16way); arch/x86/crypto/camellia_aesni_avx_glue.c:asmlinkage void camellia_ecb_dec_16way(const void *ctx, u8 *dst, const u8 *src); arch/x86/crypto/camellia_aesni_avx_glue.c:EXPORT_SYMBOL_GPL(camellia_ecb_dec_16way); arch/x86/crypto/camellia_aesni_avx_glue.c:asmlinkage void camellia_cbc_dec_16way(const void *ctx, u8 *dst, const u8 *src); arch/x86/crypto/camellia_aesni_avx_glue.c:EXPORT_SYMBOL_GPL(camellia_cbc_dec_16way); arch/x86/crypto/twofish-x86_64-asm_64-3way.S:SYM_FUNC_START(__twofish_enc_blk_3way) arch/x86/crypto/twofish.h:asmlinkage void __twofish_enc_blk_3way(const void *ctx, u8 *dst, const u8 *src, arch/x86/crypto/twofish_glue_3way.c:EXPORT_SYMBOL_GPL(__twofish_enc_blk_3way); A few of the x86 asm functions used by C code are not referenced with asmlinkage like all the others. They're not EXPORTed, though, so whether they're indirectly used can be determined. u32 crc32_pclmul_le_16(unsigned char const *buffer, size_t len, u32 crc32); void clmul_ghash_mul(char *dst, const u128 *shash); void clmul_ghash_update(char *dst, const char *src, unsigned int srclen, const u128 *shash);
On Fri, Nov 18, 2022 at 1:04 AM Eric Biggers <ebiggers@kernel.org> wrote: > > This series fixes some crashes when CONFIG_CFI_CLANG (Control Flow > Integrity) is enabled, with the new CFI implementation that was merged > in 6.1 and is supported on x86. Some of them were unconditional > crashes, while others depended on whether the compiler optimized out the > indirect calls or not. This series also simplifies some code that was > intended to work around limitations of the old CFI implementation and is > unnecessary for the new CFI implementation. > > Eric Biggers (11): > crypto: x86/aegis128 - fix crash with CFI enabled > crypto: x86/aria - fix crash with CFI enabled > crypto: x86/nhpoly1305 - eliminate unnecessary CFI wrappers > crypto: x86/sha1 - fix possible crash with CFI enabled > crypto: x86/sha256 - fix possible crash with CFI enabled > crypto: x86/sha512 - fix possible crash with CFI enabled > crypto: x86/sm3 - fix possible crash with CFI enabled > crypto: arm64/nhpoly1305 - eliminate unnecessary CFI wrapper > crypto: arm64/sm3 - fix possible crash with CFI enabled > crypto: arm/nhpoly1305 - eliminate unnecessary CFI wrapper > Revert "crypto: shash - avoid comparing pointers to exported functions > under CFI" Thanks for the patches, Eric! These look good to me. Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Sami
On Fri, Nov 18, 2022 at 03:43:55PM +0000, Elliott, Robert (Servers) wrote: > > > -----Original Message----- > > From: Eric Biggers <ebiggers@kernel.org> > > Sent: Friday, November 18, 2022 3:02 AM > > To: linux-crypto@vger.kernel.org > > Cc: x86@kernel.org; linux-arm-kernel@lists.infradead.org; Sami Tolvanen > > <samitolvanen@google.com> > > Subject: [PATCH 0/11] crypto: CFI fixes > > > > This series fixes some crashes when CONFIG_CFI_CLANG (Control Flow > > Integrity) is enabled, with the new CFI implementation that was merged > > in 6.1 and is supported on x86. Some of them were unconditional > > crashes, while others depended on whether the compiler optimized out the > > indirect calls or not. This series also simplifies some code that was > > intended to work around limitations of the old CFI implementation and is > > unnecessary for the new CFI implementation. > > Some of the x86 modules EXPORT their asm functions. Does that leave them > at risk of being called indirectly? > > arch/x86/crypto/camellia-aesni-avx-asm_64.S:SYM_FUNC_START(camellia_ecb_dec_16way) > arch/x86/crypto/camellia-aesni-avx-asm_64.S:SYM_FUNC_START(camellia_ecb_enc_16way) > arch/x86/crypto/camellia-aesni-avx-asm_64.S:SYM_FUNC_START(camellia_cbc_dec_16way) > arch/x86/crypto/camellia_aesni_avx_glue.c:asmlinkage void camellia_ecb_enc_16way(const void *ctx, u8 *dst, const u8 *src); > arch/x86/crypto/camellia_aesni_avx_glue.c:EXPORT_SYMBOL_GPL(camellia_ecb_enc_16way); > arch/x86/crypto/camellia_aesni_avx_glue.c:asmlinkage void camellia_ecb_dec_16way(const void *ctx, u8 *dst, const u8 *src); > arch/x86/crypto/camellia_aesni_avx_glue.c:EXPORT_SYMBOL_GPL(camellia_ecb_dec_16way); > arch/x86/crypto/camellia_aesni_avx_glue.c:asmlinkage void camellia_cbc_dec_16way(const void *ctx, u8 *dst, const u8 *src); > arch/x86/crypto/camellia_aesni_avx_glue.c:EXPORT_SYMBOL_GPL(camellia_cbc_dec_16way); > > arch/x86/crypto/twofish-x86_64-asm_64-3way.S:SYM_FUNC_START(__twofish_enc_blk_3way) > arch/x86/crypto/twofish.h:asmlinkage void __twofish_enc_blk_3way(const void *ctx, u8 *dst, const u8 *src, > arch/x86/crypto/twofish_glue_3way.c:EXPORT_SYMBOL_GPL(__twofish_enc_blk_3way); No, that doesn't matter at all. Whether a symbol is exported or not just has to do with how the code is divided into modules. It doesn't have anything to do with indirect calls. > A few of the x86 asm functions used by C code are not referenced with > asmlinkage like all the others. They're not EXPORTed, though, so whether > they're indirectly used can be determined. > > u32 crc32_pclmul_le_16(unsigned char const *buffer, size_t len, u32 crc32); > > void clmul_ghash_mul(char *dst, const u128 *shash); > > void clmul_ghash_update(char *dst, const char *src, unsigned int srclen, > const u128 *shash); No, the above functions are only called directly. I did do another search and found that some of the sm4 functions are called indirectly, though, so I'll send out an updated patchset that fixes those too. - Eric
> arch/x86/crypto/twofish_glue_3way.c:EXPORT_SYMBOL_GPL(__twofish_enc_blk_3w > ay); > > No, that doesn't matter at all. Whether a symbol is exported or not just > has to do with how the code is divided into modules. It doesn't have > anything to do with indirect calls. I thought that makes them available to external modules, and there is no control over how they use them.
On Fri, Nov 18, 2022 at 07:14:13PM +0000, Elliott, Robert (Servers) wrote: > > > arch/x86/crypto/twofish_glue_3way.c:EXPORT_SYMBOL_GPL(__twofish_enc_blk_3w > > ay); > > > > No, that doesn't matter at all. Whether a symbol is exported or not just > > has to do with how the code is divided into modules. It doesn't have > > anything to do with indirect calls. > > I thought that makes them available to external modules, and there is no > control over how they use them. > The upstream kernel doesn't support out of tree modules. In the highly unlikely event that someone wants to make these low-level implementation details available to out of tree modules, they can just patch the kernel themselves. - Eric
This series fixes some crashes when CONFIG_CFI_CLANG (Control Flow Integrity) is enabled, with the new CFI implementation that was merged in 6.1 and is supported on x86. Some of them were unconditional crashes, while others depended on whether the compiler optimized out the indirect calls or not. This series also simplifies some code that was intended to work around limitations of the old CFI implementation and is unnecessary for the new CFI implementation. Eric Biggers (11): crypto: x86/aegis128 - fix crash with CFI enabled crypto: x86/aria - fix crash with CFI enabled crypto: x86/nhpoly1305 - eliminate unnecessary CFI wrappers crypto: x86/sha1 - fix possible crash with CFI enabled crypto: x86/sha256 - fix possible crash with CFI enabled crypto: x86/sha512 - fix possible crash with CFI enabled crypto: x86/sm3 - fix possible crash with CFI enabled crypto: arm64/nhpoly1305 - eliminate unnecessary CFI wrapper crypto: arm64/sm3 - fix possible crash with CFI enabled crypto: arm/nhpoly1305 - eliminate unnecessary CFI wrapper Revert "crypto: shash - avoid comparing pointers to exported functions under CFI" arch/arm/crypto/nh-neon-core.S | 2 +- arch/arm/crypto/nhpoly1305-neon-glue.c | 11 ++--------- arch/arm64/crypto/nh-neon-core.S | 5 +++-- arch/arm64/crypto/nhpoly1305-neon-glue.c | 11 ++--------- arch/arm64/crypto/sm3-neon-core.S | 4 ++-- arch/x86/crypto/aegis128-aesni-asm.S | 9 +++++---- arch/x86/crypto/aria-aesni-avx-asm_64.S | 13 +++++++------ arch/x86/crypto/nh-avx2-x86_64.S | 5 +++-- arch/x86/crypto/nh-sse2-x86_64.S | 5 +++-- arch/x86/crypto/nhpoly1305-avx2-glue.c | 11 ++--------- arch/x86/crypto/nhpoly1305-sse2-glue.c | 11 ++--------- arch/x86/crypto/sha1_ni_asm.S | 3 ++- arch/x86/crypto/sha1_ssse3_asm.S | 3 ++- arch/x86/crypto/sha256-avx-asm.S | 3 ++- arch/x86/crypto/sha256-avx2-asm.S | 3 ++- arch/x86/crypto/sha256-ssse3-asm.S | 3 ++- arch/x86/crypto/sha256_ni_asm.S | 3 ++- arch/x86/crypto/sha512-avx-asm.S | 3 ++- arch/x86/crypto/sha512-avx2-asm.S | 3 ++- arch/x86/crypto/sha512-ssse3-asm.S | 3 ++- arch/x86/crypto/sm3-avx-asm_64.S | 3 ++- crypto/shash.c | 18 +++--------------- include/crypto/internal/hash.h | 8 +++++++- 23 files changed, 62 insertions(+), 81 deletions(-) base-commit: 557ffd5a4726f8b6f0dd1d4b632ae02c1c063233