Re: [PATCH v4 3/8] crypto: x86/camellia: Use new glue function macros

[Eric Biggers]

On Tue, Nov 12, 2019 at 03:41:52AM +0100, Stephan Müller wrote:
> Am Montag, 11. November 2019, 22:45:47 CET schrieb Kees Cook:
> 
> Hi Kees,
> 
> > Convert to function declaration macros from function prototype casts
> > to avoid triggering Control-Flow Integrity checks during indirect function
> > calls.
> > 
> > Co-developed-by: João Moreira <joao.moreira@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > ---
> >  arch/x86/crypto/camellia_aesni_avx2_glue.c | 73 +++++++++-------------
> >  arch/x86/crypto/camellia_aesni_avx_glue.c  | 63 +++++++------------
> >  arch/x86/crypto/camellia_glue.c            | 29 +++------
> >  arch/x86/include/asm/crypto/camellia.h     | 58 ++++-------------
> >  4 files changed, 74 insertions(+), 149 deletions(-)
> > 
> > diff --git a/arch/x86/crypto/camellia_aesni_avx2_glue.c
> > b/arch/x86/crypto/camellia_aesni_avx2_glue.c index
> > a4f00128ea55..e32b4ded3b4e 100644
> > --- a/arch/x86/crypto/camellia_aesni_avx2_glue.c
> > +++ b/arch/x86/crypto/camellia_aesni_avx2_glue.c
> > @@ -19,20 +19,12 @@
> >  #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);
> 
> Could you please help me understand the following: the CBC (and other) macros 
> use an u128 pointer. This (and other) existing function prototypes however use 
> u8 pointers. With the existing code, a caller may use an u8 pointer. By using 
> the new macro, there is now an implicit cast from u8 to u128 pointers.
> 
> So, in theory the current use cases of these functions could use data pointers 
> that may not be aligned to 128 bit boundaries.
> 
> How did you conclude that the now implicit casting from u8 to u128 is correct 
> in all use cases for all modified function prototypes?
> 
> Thanks a lot.
> 
> > -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);
> > +CRYPTO_FUNC(camellia_ecb_enc_32way);
> > +CRYPTO_FUNC(camellia_ecb_dec_32way);
> > +CRYPTO_FUNC_CBC(camellia_cbc_dec_32way);
> > +CRYPTO_FUNC_CTR(camellia_ctr_32way);
> > +CRYPTO_FUNC_XTS(camellia_xts_enc_32way);
> > +CRYPTO_FUNC_XTS(camellia_xts_dec_32way);

None of the x86 crypto algorithms except gcm(aes) set an alignmask, so there's
no alignment guarantee at all.  So the types really should be u8, not u128.  Can
you please keep the types as u8?  You can just change the types of the
common_glue*_t functions to take u8, and add the needed u8 casts in
glue_helper.c.  (glue_helper.c really shouldn't be using u128 pointers itself
either, but that can be fixed later.)

Also, I don't see the point of the macros, other than to obfuscate things.  To
keep things straightforward, I think we should keep the explicit function
prototypes for each algorithm.

Also, the CBC function wrapping is unneeded if the types are all made u8.

- Eric