| Message ID | 20180415041542.5364-2-jmoreira@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Joao Moreira <jmoreira@suse.de> wrote: > Convert the use of 'struct camellia_ctx *' to 'void *' in prototypes of > functions which are referenced through 'struct common_glue_func_entry', > making their prototypes match those of this struct and, consequently, > turning them compatible with CFI requirements. > > Whenever needed, cast 'void *' to 'struct camellia_ctx *'. > +static inline void camellia_enc_blk(void *ctx, u8 *dst, const u8 *src) > { > - __camellia_enc_blk(ctx, dst, src, false); > + __camellia_enc_blk((struct camellia_ctx *) ctx, dst, src, false); > } I don't see how this is an improvement: instead of having a proper type there's now an opaque type and an ugly (and dangerous) type cast. What does "compatible with CFI requirements" mean? Thanks, Ingo
> >> +static inline void camellia_enc_blk(void *ctx, u8 *dst, const u8 *src) >> { >> - __camellia_enc_blk(ctx, dst, src, false); >> + __camellia_enc_blk((struct camellia_ctx *) ctx, dst, src, false); >> } > > I don't see how this is an improvement: instead of having a proper type there's > now an opaque type and an ugly (and dangerous) type cast. > > What does "compatible with CFI requirements" mean? For "compatible with CFI requirements", I meant: given a set of functions that are invoked through a given set of pointers, all functions and pointers in these sets have the same prototype. I added a note about this CFI heuristic in the cover letter, but I guess I should have been clearer there and added something more substantial to the commit message. Regarding the changes, this was the most straight-forward way I found to make the sources compliant with the above, not requiring deeper semantic changes to the underneath invoking code. On the other hand, I agree that there is a collateral damage and that an ideal fix would be the other way around -- removing the opaque type from the pointer instead of adding it to the functions. I'll try to think of another way and send if I come to something. Tks, João.
On Sun, Apr 15, 2018 at 11:10 PM, Ingo Molnar <mingo@kernel.org> wrote: > > * Joao Moreira <jmoreira@suse.de> wrote: > >> Convert the use of 'struct camellia_ctx *' to 'void *' in prototypes of >> functions which are referenced through 'struct common_glue_func_entry', >> making their prototypes match those of this struct and, consequently, >> turning them compatible with CFI requirements. >> >> Whenever needed, cast 'void *' to 'struct camellia_ctx *'. > >> +static inline void camellia_enc_blk(void *ctx, u8 *dst, const u8 *src) >> { >> - __camellia_enc_blk(ctx, dst, src, false); >> + __camellia_enc_blk((struct camellia_ctx *) ctx, dst, src, false); >> } > > I don't see how this is an improvement: instead of having a proper type there's > now an opaque type and an ugly (and dangerous) type cast. I agree with your instinct, as the existing best-practice in the kernel is to always use correct types and avoid casts to let the compiler check things, avoid nasty surprises, etc, but for callbacks with context pointers, we're already just hiding the casts in various places. For example, even without this patch: typedef void (*common_glue_func_t)(void *ctx, u8 *dst, const u8 *src); #define GLUE_FUNC_CAST(fn) ((common_glue_func_t)(fn)) static const struct common_glue_ctx camellia_enc = { ... .funcs = { { ... .num_blocks = 1, .fn_u = { .ecb = GLUE_FUNC_CAST(camellia_enc_blk) } } } }; While it is nicely wrapped up in macros, this is actually casting away the _entire_ function prototype, not just the first argument. The "camellia_enc_blk" function could point to anything (even "void (*func)(void)") and the compiler wouldn't complain. And this was done just to mask the ctx argument. > What does "compatible with CFI requirements" mean? As Joao mentioned, he wrote this up pretty well in his 0/n patch: https://lkml.kernel.org/r/20180415041542.5364-1-jmoreira@suse.de To further clarify, fine-grained function-prototype-checking CFI makes sure that indirect function call sites only call functions with a matching expected prototype (to protect against having function pointers rewritten during an attack, etc). In this case, callers of .fn_u.ecb() expect the target function to have the prototype "void (*func)(void *ctx, u8 *dst, const u8 *src)" (as defined by struct common_glue_ctx), however, camellia_enc_blk() is "void (*func)(struct camellia_ctx *ctx, u8 *dst, const u8 *src)" and will trip the CFI check. If we rearrange our macro magic to defining the callbacks rather than defining the function pointers, I think we'll gain several things: - we will cast only the ctx argument instead of the entire function prototype - we'll retain (and improve) source-code robustness against generally miscasting - CFI will be happy So, instead of the original series, how about something like this: Switch function pointers to not use the function cast, and define a new GLUE_FUNC macro that kinda looks like the tricks used for syscalls: static const struct common_glue_ctx camellia_enc = { ... .funcs = { { ... .num_blocks = 1, .fn_u = { .ecb = camellia_enc_blk } } } }; #define GLUE_FUNC(func, context) \ static inline void func(void *ctx, u8 *dst, const u8 *src) \ { __##func((context)ctx, dst, src); } \ __##func(context ctx, u8 *dst, const u8 *src) GLUE_FUNC(camellia_enc_blk, struct camellia_ctx *) { ...original function... } -Kees
On Wed, 18 Apr 2018, Kees Cook wrote: > So, instead of the original series, how about something like this: > > Switch function pointers to not use the function cast, and define a > new GLUE_FUNC macro that kinda looks like the tricks used for > syscalls: > > static const struct common_glue_ctx camellia_enc = { > ... > .funcs = { { > ... > .num_blocks = 1, > .fn_u = { .ecb = camellia_enc_blk } > } } > }; > > #define GLUE_FUNC(func, context) \ > static inline void func(void *ctx, u8 *dst, const u8 *src) \ > { __##func((context)ctx, dst, src); } \ > __##func(context ctx, u8 *dst, const u8 *src) > > GLUE_FUNC(camellia_enc_blk, struct camellia_ctx *) > { > ...original function... > } I was about to suggest this before I read to the end of the mail. Yes, that is much more palatable. Thanks, tglx
diff --git a/arch/x86/crypto/camellia_aesni_avx2_glue.c b/arch/x86/crypto/camellia_aesni_avx2_glue.c index 60907c139c4e..6fe248ee5efa 100644 --- a/arch/x86/crypto/camellia_aesni_avx2_glue.c +++ b/arch/x86/crypto/camellia_aesni_avx2_glue.c @@ -27,20 +27,17 @@ #define CAMELLIA_AESNI_AVX2_PARALLEL_BLOCKS 32 /* 32-way AVX2/AES-NI parallel cipher functions */ -asmlinkage void camellia_ecb_enc_32way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); -asmlinkage void camellia_ecb_dec_32way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); - -asmlinkage void camellia_cbc_dec_32way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); -asmlinkage void camellia_ctr_32way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); - -asmlinkage void camellia_xts_enc_32way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); -asmlinkage void camellia_xts_dec_32way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); +asmlinkage void camellia_ecb_enc_32way(void *ctx, u8 *dst, const u8 *src); +asmlinkage void camellia_ecb_dec_32way(void *ctx, u8 *dst, const u8 *src); + +asmlinkage void camellia_cbc_dec_32way(void *ctx, u8 *dst, const u8 *src); +asmlinkage void camellia_ctr_32way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); + +asmlinkage void camellia_xts_enc_32way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); +asmlinkage void camellia_xts_dec_32way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); static const struct common_glue_ctx camellia_enc = { .num_funcs = 4, diff --git a/arch/x86/crypto/camellia_aesni_avx_glue.c b/arch/x86/crypto/camellia_aesni_avx_glue.c index d96429da88eb..3ccfd75b4af4 100644 --- a/arch/x86/crypto/camellia_aesni_avx_glue.c +++ b/arch/x86/crypto/camellia_aesni_avx_glue.c @@ -26,28 +26,25 @@ #define CAMELLIA_AESNI_PARALLEL_BLOCKS 16 /* 16-way parallel cipher functions (avx/aes-ni) */ -asmlinkage void camellia_ecb_enc_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); +asmlinkage void camellia_ecb_enc_16way(void *ctx, u8 *dst, const u8 *src); EXPORT_SYMBOL_GPL(camellia_ecb_enc_16way); -asmlinkage void camellia_ecb_dec_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); +asmlinkage void camellia_ecb_dec_16way(void *ctx, u8 *dst, const u8 *src); EXPORT_SYMBOL_GPL(camellia_ecb_dec_16way); -asmlinkage void camellia_cbc_dec_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); +asmlinkage void camellia_cbc_dec_16way(void *ctx, u8 *dst, const u8 *src); EXPORT_SYMBOL_GPL(camellia_cbc_dec_16way); -asmlinkage void camellia_ctr_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); +asmlinkage void camellia_ctr_16way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); EXPORT_SYMBOL_GPL(camellia_ctr_16way); -asmlinkage void camellia_xts_enc_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); +asmlinkage void camellia_xts_enc_16way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); EXPORT_SYMBOL_GPL(camellia_xts_enc_16way); -asmlinkage void camellia_xts_dec_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); +asmlinkage void camellia_xts_dec_16way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); EXPORT_SYMBOL_GPL(camellia_xts_dec_16way); void camellia_xts_enc(void *ctx, u128 *dst, const u128 *src, le128 *iv) diff --git a/arch/x86/crypto/camellia_glue.c b/arch/x86/crypto/camellia_glue.c index af4840ab2a3d..0942b3998de5 100644 --- a/arch/x86/crypto/camellia_glue.c +++ b/arch/x86/crypto/camellia_glue.c @@ -39,16 +39,14 @@ asmlinkage void __camellia_enc_blk(struct camellia_ctx *ctx, u8 *dst, const u8 *src, bool xor); EXPORT_SYMBOL_GPL(__camellia_enc_blk); -asmlinkage void camellia_dec_blk(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); +asmlinkage void camellia_dec_blk(void *ctx, u8 *dst, const u8 *src); EXPORT_SYMBOL_GPL(camellia_dec_blk); /* 2-way parallel cipher functions */ asmlinkage void __camellia_enc_blk_2way(struct camellia_ctx *ctx, u8 *dst, const u8 *src, bool xor); EXPORT_SYMBOL_GPL(__camellia_enc_blk_2way); -asmlinkage void camellia_dec_blk_2way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); +asmlinkage void camellia_dec_blk_2way(void *ctx, u8 *dst, const u8 *src); EXPORT_SYMBOL_GPL(camellia_dec_blk_2way); static void camellia_encrypt(struct crypto_tfm *tfm, u8 *dst, const u8 *src) diff --git a/arch/x86/include/asm/crypto/camellia.h b/arch/x86/include/asm/crypto/camellia.h index 10f8d590bcfe..974bea896d96 100644 --- a/arch/x86/include/asm/crypto/camellia.h +++ b/arch/x86/include/asm/crypto/camellia.h @@ -40,35 +40,29 @@ extern int xts_camellia_setkey(struct crypto_tfm *tfm, const u8 *key, /* regular block cipher functions */ asmlinkage void __camellia_enc_blk(struct camellia_ctx *ctx, u8 *dst, const u8 *src, bool xor); -asmlinkage void camellia_dec_blk(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); +asmlinkage void camellia_dec_blk(void *ctx, u8 *dst, const u8 *src); /* 2-way parallel cipher functions */ asmlinkage void __camellia_enc_blk_2way(struct camellia_ctx *ctx, u8 *dst, const u8 *src, bool xor); -asmlinkage void camellia_dec_blk_2way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); +asmlinkage void camellia_dec_blk_2way(void *ctx, u8 *dst, const u8 *src); /* 16-way parallel cipher functions (avx/aes-ni) */ -asmlinkage void camellia_ecb_enc_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); -asmlinkage void camellia_ecb_dec_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); - -asmlinkage void camellia_cbc_dec_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src); -asmlinkage void camellia_ctr_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); - -asmlinkage void camellia_xts_enc_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); -asmlinkage void camellia_xts_dec_16way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src, le128 *iv); - -static inline void camellia_enc_blk(struct camellia_ctx *ctx, u8 *dst, - const u8 *src) +asmlinkage void camellia_ecb_enc_16way(void *ctx, u8 *dst, const u8 *src); +asmlinkage void camellia_ecb_dec_16way(void *ctx, u8 *dst, const u8 *src); + +asmlinkage void camellia_cbc_dec_16way(void *ctx, u8 *dst, const u8 *src); +asmlinkage void camellia_ctr_16way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); + +asmlinkage void camellia_xts_enc_16way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); +asmlinkage void camellia_xts_dec_16way(void *ctx, u8 *dst, const u8 *src, + le128 *iv); + +static inline void camellia_enc_blk(void *ctx, u8 *dst, const u8 *src) { - __camellia_enc_blk(ctx, dst, src, false); + __camellia_enc_blk((struct camellia_ctx *) ctx, dst, src, false); } static inline void camellia_enc_blk_xor(struct camellia_ctx *ctx, u8 *dst, @@ -77,10 +71,9 @@ static inline void camellia_enc_blk_xor(struct camellia_ctx *ctx, u8 *dst, __camellia_enc_blk(ctx, dst, src, true); } -static inline void camellia_enc_blk_2way(struct camellia_ctx *ctx, u8 *dst, - const u8 *src) +static inline void camellia_enc_blk_2way(void *ctx, u8 *dst, const u8 *src) { - __camellia_enc_blk_2way(ctx, dst, src, false); + __camellia_enc_blk_2way((struct camellia_ctx *) ctx, dst, src, false); } static inline void camellia_enc_blk_xor_2way(struct camellia_ctx *ctx, u8 *dst,
Convert the use of 'struct camellia_ctx *' to 'void *' in prototypes of functions which are referenced through 'struct common_glue_func_entry', making their prototypes match those of this struct and, consequently, turning them compatible with CFI requirements. Whenever needed, cast 'void *' to 'struct camellia_ctx *'. Signed-off-by: João Moreira <jmoreira@suse.de> --- arch/x86/crypto/camellia_aesni_avx2_glue.c | 25 ++++++++--------- arch/x86/crypto/camellia_aesni_avx_glue.c | 21 +++++++-------- arch/x86/crypto/camellia_glue.c | 6 ++--- arch/x86/include/asm/crypto/camellia.h | 43 +++++++++++++----------------- 4 files changed, 40 insertions(+), 55 deletions(-)