| Message ID | 20190507161321.34611-1-keescook@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | crypto: x86: Fix indirect function call casts | expand |
On Tue, May 07, 2019 at 09:13:14AM -0700, Kees Cook wrote: > It is possible to indirectly invoke functions with prototypes that do > not match those of the respectively used function pointers by using void > types or casts. This feature is frequently used as a way of relaxing > function invocation, making it possible that different data structures > are passed to different functions through the same pointer. > > Despite the benefits, this can lead to a situation where functions with a > given prototype are invoked by pointers with a different prototype. This > is undesirable as it may prevent the use of heuristics such as prototype > matching-based Control-Flow Integrity, which can be used to prevent > ROP-based attacks. > > One way of fixing this situation is through the use of inline helper > functions with prototypes that match the one in the respective invoking > pointer. > > Given the above, the current efforts to improve the Linux security, > and the upcoming kernel support to compilers with CFI features, this > creates macros to be used to build the needed function definitions, > to be used in camellia, cast6, serpent, twofish, and aesni. So why not change the function prototypes to be compatible with common_glue_*_t instead, rather than wrapping them with another layer of functions? Is it because indirect calls into asm code won't be allowed with CFI? > > -Kees (and Joao) > > v3: > - no longer RFC > - consolidate macros into glue_helper.h > - include aesni which was using casts as well > - remove XTS_TWEAK_CAST while we're at it > > v2: > - update cast macros for clarity > > v1: > - initial prototype > > Joao Moreira (4): > crypto: x86/crypto: Use new glue function macros This one should be "x86/serpent", not "x86/crypto". > crypto: x86/camellia: Use new glue function macros > crypto: x86/twofish: Use new glue function macros > crypto: x86/cast6: Use new glue function macros > > Kees Cook (3): > crypto: x86/glue_helper: Add static inline function glue macros > crypto: x86/aesni: Use new glue function macros > crypto: x86/glue_helper: Remove function prototype cast helpers > > arch/x86/crypto/aesni-intel_glue.c | 31 ++++----- > arch/x86/crypto/camellia_aesni_avx2_glue.c | 73 +++++++++------------- > arch/x86/crypto/camellia_aesni_avx_glue.c | 63 +++++++------------ > arch/x86/crypto/camellia_glue.c | 21 +++---- > arch/x86/crypto/cast6_avx_glue.c | 65 +++++++++---------- > arch/x86/crypto/serpent_avx2_glue.c | 65 +++++++++---------- > arch/x86/crypto/serpent_avx_glue.c | 58 ++++++----------- > arch/x86/crypto/serpent_sse2_glue.c | 27 +++++--- > arch/x86/crypto/twofish_avx_glue.c | 71 ++++++++------------- > arch/x86/crypto/twofish_glue_3way.c | 28 ++++----- > arch/x86/include/asm/crypto/camellia.h | 64 ++++++------------- > arch/x86/include/asm/crypto/glue_helper.h | 34 ++++++++-- > arch/x86/include/asm/crypto/serpent-avx.h | 28 ++++----- > arch/x86/include/asm/crypto/twofish.h | 22 ++++--- > include/crypto/xts.h | 2 - > 15 files changed, 283 insertions(+), 369 deletions(-) > > -- > 2.17.1 >
On Tue, May 7, 2019 at 10:00 AM Eric Biggers <ebiggers@kernel.org> wrote: > > Given the above, the current efforts to improve the Linux security, > > and the upcoming kernel support to compilers with CFI features, this > > creates macros to be used to build the needed function definitions, > > to be used in camellia, cast6, serpent, twofish, and aesni. > > So why not change the function prototypes to be compatible with common_glue_*_t > instead, rather than wrapping them with another layer of functions? Is it > because indirect calls into asm code won't be allowed with CFI? I don't know why they're not that way to begin with. But given that the casting was already happening, this is just moving it to a place where CFI won't be angry. :) > > crypto: x86/crypto: Use new glue function macros > > This one should be "x86/serpent", not "x86/crypto". Oops, yes, that's my typo. I'll fix for v4. Do the conversions themselves look okay (the changes are pretty mechanical)? If so, Herbert, do you want a v4 with the typo fix, or do you want to fix that up yourself? Thanks!
On Tue, May 07, 2019 at 02:07:51PM -0700, Kees Cook wrote: > On Tue, May 7, 2019 at 10:00 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > Given the above, the current efforts to improve the Linux security, > > > and the upcoming kernel support to compilers with CFI features, this > > > creates macros to be used to build the needed function definitions, > > > to be used in camellia, cast6, serpent, twofish, and aesni. > > > > So why not change the function prototypes to be compatible with common_glue_*_t > > instead, rather than wrapping them with another layer of functions? Is it > > because indirect calls into asm code won't be allowed with CFI? > > I don't know why they're not that way to begin with. But given that > the casting was already happening, this is just moving it to a place > where CFI won't be angry. :) > > > > crypto: x86/crypto: Use new glue function macros > > > > This one should be "x86/serpent", not "x86/crypto". > > Oops, yes, that's my typo. I'll fix for v4. Do the conversions > themselves look okay (the changes are pretty mechanical)? If so, > Herbert, do you want a v4 with the typo fix, or do you want to fix > that up yourself? > > Thanks! > I don't know yet. It's difficult to read the code with 2 layers of macros. Hence why I asked why you didn't just change the prototypes to be compatible. - Eric
On Tue, May 07, 2019 at 02:50:46PM -0700, Eric Biggers wrote: > > I don't know yet. It's difficult to read the code with 2 layers of macros. > > Hence why I asked why you didn't just change the prototypes to be compatible. I agree. Kees, since you're changing this anyway please make it look better not worse. Thanks,
On Wed, May 8, 2019 at 6:36 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Tue, May 07, 2019 at 02:50:46PM -0700, Eric Biggers wrote: > > > > I don't know yet. It's difficult to read the code with 2 layers of macros. > > > > Hence why I asked why you didn't just change the prototypes to be compatible. > > I agree. Kees, since you're changing this anyway please make it > look better not worse. Do you mean I should use the typedefs in the new macros? I'm not aware of a way to use a typedef to declare a function body, so I had to repeat them. I'm open to suggestions! As far as "fixing the prototypes", the API is agnostic of the context type, and uses void *. And also it provides a way to call the same function with different pointer types on the other arguments: For example, quoting the existing code: asmlinkage void twofish_dec_blk(struct twofish_ctx *ctx, u8 *dst, const u8 *src); Which is used for ecb and cbc: #define GLUE_FUNC_CAST(fn) ((common_glue_func_t)(fn)) #define GLUE_CBC_FUNC_CAST(fn) ((common_glue_cbc_func_t)(fn)) ... static const struct common_glue_ctx twofish_dec = { ... .fn_u = { .ecb = GLUE_FUNC_CAST(twofish_dec_blk) } static const struct common_glue_ctx twofish_dec_cbc = { ... .fn_u = { .cbc = GLUE_CBC_FUNC_CAST(twofish_dec_blk) } which have different prototypes: typedef void (*common_glue_func_t)(void *ctx, u8 *dst, const u8 *src); typedef void (*common_glue_cbc_func_t)(void *ctx, u128 *dst, const u128 *src); ... struct common_glue_func_entry { unsigned int num_blocks; /* number of blocks that @fn will process */ union { common_glue_func_t ecb; common_glue_cbc_func_t cbc; common_glue_ctr_func_t ctr; common_glue_xts_func_t xts; } fn_u; }; What CFI dislikes is calling a func(void *ctx, ...) when the actual function is, for example, func(struct twofish_ctx *ctx, ...). This needs to be fixed at the call site, not the static initializers, and since the call site is void, there needs to be a static inline that will satisfy the types. I'm open to suggestions! :) Thanks,
On Wed, May 08, 2019 at 02:08:25PM -0700, Kees Cook wrote: > > For example, quoting the existing code: > > asmlinkage void twofish_dec_blk(struct twofish_ctx *ctx, u8 *dst, > const u8 *src); So just make it asmlinkage void twofish_dec_blk(void *ctx, u8 *dst, const u8 *src); and you won't need any of these casts. Cheers,
On Tue, May 07, 2019 at 09:13:14AM -0700, Kees Cook wrote: > It is possible to indirectly invoke functions with prototypes that do > not match those of the respectively used function pointers by using void > types or casts. This feature is frequently used as a way of relaxing > function invocation, making it possible that different data structures > are passed to different functions through the same pointer. > > Despite the benefits, this can lead to a situation where functions with a > given prototype are invoked by pointers with a different prototype. This > is undesirable as it may prevent the use of heuristics such as prototype > matching-based Control-Flow Integrity, which can be used to prevent > ROP-based attacks. > > One way of fixing this situation is through the use of inline helper > functions with prototypes that match the one in the respective invoking > pointer. > > Given the above, the current efforts to improve the Linux security, > and the upcoming kernel support to compilers with CFI features, this > creates macros to be used to build the needed function definitions, > to be used in camellia, cast6, serpent, twofish, and aesni. > > -Kees (and Joao) Did you try enabling -Wcast-function-type? It seems you missed some cases: arch/x86/crypto/sha256_ssse3_glue.c: In function ‘sha256_update’: arch/x86/crypto/sha256_ssse3_glue.c:62:10: warning: cast between incompatible function types from ‘void (*)(u32 *, const char *, u64)’ {aka ‘void (*)(unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha256_state *, const u8 *, int)’ {aka ‘void (*)(struct sha256_state *, const unsigned char *, int)’} [-Wcast-function-type] (sha256_block_fn *)sha256_xform); ^ arch/x86/crypto/sha256_ssse3_glue.c: In function ‘sha256_finup’: arch/x86/crypto/sha256_ssse3_glue.c:77:11: warning: cast between incompatible function types from ‘void (*)(u32 *, const char *, u64)’ {aka ‘void (*)(unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha256_state *, const u8 *, int)’ {aka ‘void (*)(struct sha256_state *, const unsigned char *, int)’} [-Wcast-function-type] (sha256_block_fn *)sha256_xform); ^ arch/x86/crypto/sha256_ssse3_glue.c:78:32: warning: cast between incompatible function types from ‘void (*)(u32 *, const char *, u64)’ {aka ‘void (*)(unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha256_state *, const u8 *, int)’ {aka ‘void (*)(struct sha256_state *, const unsigned char *, int)’} [-Wcast-function-type] sha256_base_do_finalize(desc, (sha256_block_fn *)sha256_xform); ^ CC arch/x86/crypto/sha512_ssse3_glue.o arch/x86/crypto/sha512_ssse3_glue.c: In function ‘sha512_update’: arch/x86/crypto/sha512_ssse3_glue.c:61:10: warning: cast between incompatible function types from ‘void (*)(u64 *, const char *, u64)’ {aka ‘void (*)(long long unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha512_state *, const u8 *, int)’ {aka ‘void (*)(struct sha512_state *, const unsigned char *, int)’} [-Wcast-function-type] (sha512_block_fn *)sha512_xform); ^ arch/x86/crypto/sha512_ssse3_glue.c: In function ‘sha512_finup’: arch/x86/crypto/sha512_ssse3_glue.c:76:11: warning: cast between incompatible function types from ‘void (*)(u64 *, const char *, u64)’ {aka ‘void (*)(long long unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha512_state *, const u8 *, int)’ {aka ‘void (*)(struct sha512_state *, const unsigned char *, int)’} [-Wcast-function-type] (sha512_block_fn *)sha512_xform); ^ arch/x86/crypto/sha512_ssse3_glue.c:77:32: warning: cast between incompatible function types from ‘void (*)(u64 *, const char *, u64)’ {aka ‘void (*)(long long unsigned int *, const char *, long long unsigned int)’} to ‘void (*)(struct sha512_state *, const u8 *, int)’ {aka ‘void (*)(struct sha512_state *, const unsigned char *, int)’} [-Wcast-function-type] sha512_base_do_finalize(desc, (sha512_block_fn *)sha512_xform); ^
On Wed, May 08, 2019 at 02:08:25PM -0700, Kees Cook wrote: > On Wed, May 8, 2019 at 6:36 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Tue, May 07, 2019 at 02:50:46PM -0700, Eric Biggers wrote: > > > > > > I don't know yet. It's difficult to read the code with 2 layers of macros. > > > > > > Hence why I asked why you didn't just change the prototypes to be compatible. > > > > I agree. Kees, since you're changing this anyway please make it > > look better not worse. > > Do you mean I should use the typedefs in the new macros? I'm not aware > of a way to use a typedef to declare a function body, so I had to > repeat them. I'm open to suggestions! > > As far as "fixing the prototypes", the API is agnostic of the context > type, and uses void *. And also it provides a way to call the same > function with different pointer types on the other arguments: > > For example, quoting the existing code: > > asmlinkage void twofish_dec_blk(struct twofish_ctx *ctx, u8 *dst, > const u8 *src); > > Which is used for ecb and cbc: > > #define GLUE_FUNC_CAST(fn) ((common_glue_func_t)(fn)) > #define GLUE_CBC_FUNC_CAST(fn) ((common_glue_cbc_func_t)(fn)) > ... > static const struct common_glue_ctx twofish_dec = { > ... > .fn_u = { .ecb = GLUE_FUNC_CAST(twofish_dec_blk) } > > static const struct common_glue_ctx twofish_dec_cbc = { > ... > .fn_u = { .cbc = GLUE_CBC_FUNC_CAST(twofish_dec_blk) } > > which have different prototypes: > > typedef void (*common_glue_func_t)(void *ctx, u8 *dst, const u8 *src); > typedef void (*common_glue_cbc_func_t)(void *ctx, u128 *dst, const u128 *src); > ... > struct common_glue_func_entry { > unsigned int num_blocks; /* number of blocks that @fn will process */ > union { > common_glue_func_t ecb; > common_glue_cbc_func_t cbc; > common_glue_ctr_func_t ctr; > common_glue_xts_func_t xts; > } fn_u; > }; > As Herbert said, the ctx parameters could be made 'void *'. And I also asked whether indirect calls to asm code are even allowed with CFI. IIRC, the AOSP kernels have been patched to remove them from arm64. It would be helpful if you would answer that question, since it would inform the best approach here. As for the "ecb" functions taking 'u8 *' but the "cbc" ones taking 'u128 *' and the same function being used in the blocks==1 case, you could just pick one of the types to use for both. 'u8 *' probably makes more sense since both ecb and cbc operate on blocks of 16 bytes but don't interpret them as 128-bit integers. - Eric
On 5/8/19 11:04 PM, Eric Biggers wrote: > On Wed, May 08, 2019 at 02:08:25PM -0700, Kees Cook wrote: >> On Wed, May 8, 2019 at 6:36 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: >>> On Tue, May 07, 2019 at 02:50:46PM -0700, Eric Biggers wrote: >>>> >>>> I don't know yet. It's difficult to read the code with 2 layers of macros. >>>> >>>> Hence why I asked why you didn't just change the prototypes to be compatible. >>> >>> I agree. Kees, since you're changing this anyway please make it >>> look better not worse. >> >> Do you mean I should use the typedefs in the new macros? I'm not aware >> of a way to use a typedef to declare a function body, so I had to >> repeat them. I'm open to suggestions! >> >> As far as "fixing the prototypes", the API is agnostic of the context >> type, and uses void *. And also it provides a way to call the same >> function with different pointer types on the other arguments: >> >> For example, quoting the existing code: >> >> asmlinkage void twofish_dec_blk(struct twofish_ctx *ctx, u8 *dst, >> const u8 *src); >> >> Which is used for ecb and cbc: >> >> #define GLUE_FUNC_CAST(fn) ((common_glue_func_t)(fn)) >> #define GLUE_CBC_FUNC_CAST(fn) ((common_glue_cbc_func_t)(fn)) >> ... >> static const struct common_glue_ctx twofish_dec = { >> ... >> .fn_u = { .ecb = GLUE_FUNC_CAST(twofish_dec_blk) } >> >> static const struct common_glue_ctx twofish_dec_cbc = { >> ... >> .fn_u = { .cbc = GLUE_CBC_FUNC_CAST(twofish_dec_blk) } >> >> which have different prototypes: >> >> typedef void (*common_glue_func_t)(void *ctx, u8 *dst, const u8 *src); >> typedef void (*common_glue_cbc_func_t)(void *ctx, u128 *dst, const u128 *src); >> ... >> struct common_glue_func_entry { >> unsigned int num_blocks; /* number of blocks that @fn will process */ >> union { >> common_glue_func_t ecb; >> common_glue_cbc_func_t cbc; >> common_glue_ctr_func_t ctr; >> common_glue_xts_func_t xts; >> } fn_u; >> }; >> > > As Herbert said, the ctx parameters could be made 'void *'. > This is how things were done in the original patch set, but some concerns were raised about this approach: https://lkml.org/lkml/2018/4/16/74 Tks, Joao. > And I also asked whether indirect calls to asm code are even allowed with CFI. > IIRC, the AOSP kernels have been patched to remove them from arm64. It would be > helpful if you would answer that question, since it would inform the best > approach here. > > As for the "ecb" functions taking 'u8 *' but the "cbc" ones taking 'u128 *' and > the same function being used in the blocks==1 case, you could just pick one of > the types to use for both. 'u8 *' probably makes more sense since both ecb and > cbc operate on blocks of 16 bytes but don't interpret them as 128-bit integers. > > - Eric >
On Thu, May 09, 2019 at 12:12:54AM -0300, Joao Moreira wrote: > > This is how things were done in the original patch set, but some concerns > were raised about this approach: > > https://lkml.org/lkml/2018/4/16/74 No that's not what I'm suggesting at all. Just get rid of those wrapper functions and change the underlying asm functions to take a void *. Cheers,
On Wed, May 08, 2019 at 07:04:40PM -0700, Eric Biggers wrote: > And I also asked whether indirect calls to asm code are even allowed > with CFI. IIRC, the AOSP kernels have been patched to remove them from > arm64 At least with clang, indirect calls to stand-alone assembly functions trip CFI checks, which is why Android kernels use static inline stubs to convert these to direct calls instead. Sami
On Thu, May 09, 2019 at 08:38:28AM -0700, Sami Tolvanen wrote: > On Wed, May 08, 2019 at 07:04:40PM -0700, Eric Biggers wrote: > > And I also asked whether indirect calls to asm code are even allowed > > with CFI. IIRC, the AOSP kernels have been patched to remove them from > > arm64 > > At least with clang, indirect calls to stand-alone assembly functions > trip CFI checks, which is why Android kernels use static inline stubs > to convert these to direct calls instead. > > Sami Thanks Sami. Is there any way to annotate assembly functions such that they work directly with CFI? Otherwise, we need the wrapper functions. Kees and Joao, it would be helpful if you'd explain this in the patchset. - Eric
On Thu, May 09, 2019 at 10:58:23AM -0700, Eric Biggers wrote: > Is there any way to annotate assembly functions such that they work > directly with CFI? Not to my knowledge. Sami