Re: [PATCH v3 2/4] crypto: aria: do not use magic number offsets of aria_ctx

[Taehee Yoo <ap420073@gmail.com>]

Hi Elliott,
Thank you so much for your review!

On 2022. 11. 10. 오후 12:55, Elliott, Robert (Servers) wrote:
 >
 >
 >> -----Original Message-----
 >> From: Taehee Yoo <ap420073@gmail.com>
 >> Sent: Sunday, November 6, 2022 8:36 AM
 >> To: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au;
 >> davem@davemloft.net; tglx@linutronix.de; mingo@redhat.com; bp@alien8.de;
 >> dave.hansen@linux.intel.com; hpa@zytor.com;
 >> kirill.shutemov@linux.intel.com; richard@nod.at; 
viro@zeniv.linux.org.uk;
 >> sathyanarayanan.kuppuswamy@linux.intel.com; jpoimboe@kernel.org; 
Elliott,
 >> Robert (Servers) <elliott@hpe.com>; x86@kernel.org; 
jussi.kivilinna@iki.fi
 >> Cc: ap420073@gmail.com
 >> Subject: [PATCH v3 2/4] crypto: aria: do not use magic number offsets of
 >> aria_ctx
 >>
 >> aria-avx assembly code accesses members of aria_ctx with magic number
 >> offset. If the shape of struct aria_ctx is changed carelessly,
 >> aria-avx will not work.
 >> So, we need to ensure accessing members of aria_ctx with correct
 >> offset values, not with magic numbers.
 >>
 >> It adds ARIA_CTX_enc_key, ARIA_CTX_dec_key, and ARIA_CTX_rounds in the
 >> asm-offsets.c So, correct offset definitions will be generated.
 >> aria-avx assembly code can access members of aria_ctx safely with
 >> these definitions.
 >>
 >> diff --git a/arch/x86/crypto/aria-aesni-avx-asm_64.S
 > ...
 >>
 >>   #include <linux/linkage.h>
 >>   #include <asm/frame.h>
 >> -
 >> -/* struct aria_ctx: */
 >> -#define enc_key 0
 >> -#define dec_key 272
 >> -#define rounds 544
 >
 > That structure also has a key_length field after the rounds field.
 > aria_set_key() sets it, but no function ever seems to use it.
 > Perhaps that field should be removed?

Okay, I will remove that fields in a separate patch.

 >
 >> +#include <asm/asm-offsets.h>
 >
 > That makes the offsets flexible, which is good.
 >
 > The assembly code also implicitly assumes the size of each of those 
fields
 > (e.g., enc_key is 272 bytes, dec_key is 272 bytes, and rounds is 4 
bytes).
 > A BUILD_BUG_ON confirming those assumptions might be worthwhile.

You're right,
I didn't consider the size of the fields.
I will contain BUILD_BUG_ON() to check the size.

 >
 >> diff --git a/arch/x86/kernel/asm-offsets.c 
b/arch/x86/kernel/asm-offsets.c
 >> index cb50589a7102..32192a91c65b 100644
 >> --- a/arch/x86/kernel/asm-offsets.c
 >> +++ b/arch/x86/kernel/asm-offsets.c
 >> @@ -7,6 +7,7 @@
 >>   #define COMPILE_OFFSETS
 >>
 >>   #include <linux/crypto.h>
 >> +#include <crypto/aria.h>
 >
 > Is it safe to include .h files for a large number of crypto modules
 > in one C file? It seems like they could easily include naming conflicts.
 >
 > However, this set does seem to compile cleanly:
 >

Thanks for this check.
Sorry, I'm not sure about the side effects of a large number of header 
in .c file.

 > +// no .h for aegis, but ctx is simple
 > +#include <crypto/aes.h>
 > +#include <crypto/aria.h>
 > +#include <crypto/blake2s.h>
 > +#include <crypto/blowfish.h>
 > +// no .h for camellia in crypto; it is in arch/x86/crypto
 > +#include <crypto/cast5.h>
 > +#include <crypto/cast6.h>
 > +#include <crypto/chacha.h>
 > +// no .h for crc32c, crc32, crct10dif, but no ctx structure to worry 
about
 > +#include <crypto/curve25519.h>
 > +#include <crypto/des.h>
 > +#include <crypto/ghash.h>
 > +#include <crypto/nhpoly1305.h>
 > +#include <crypto/poly1305.h>
 > +#include <crypto/polyval.h>
 > +#include <crypto/serpent.h>
 > +#include <crypto/sha1.h>
 > +#include <crypto/sha2.h>
 > +#include <crypto/sm3.h>
 > +#include <crypto/twofish.h>
 >

Thanks a lot!
Taehee Yoo