Re: [PATCH 1/4] x86/crypto: camellia: Fix function prototypes
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
Re: [PATCH 1/4] x86/crypto: camellia: Fix function prototypes
On Sun, Apr 15, 2018 at 11:10 PM, Ingo Molnar wrote:
>
> * Joao Moreira 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-jmore...@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
--
Kees Cook
Pixel Security
Re: [PATCH 1/4] x86/crypto: camellia: Fix function prototypes
+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.
Re: [PATCH 1/4] x86/crypto: camellia: Fix function prototypes
* Joao Moreira 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
[PATCH 1/4] x86/crypto: camellia: Fix function prototypes
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
---
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(-)
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(__c
