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@xxxxxxxxx>
>> Sent: Sunday, November 6, 2022 8:36 AM
>> To: linux-crypto@xxxxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
>> davem@xxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx;
>> dave.hansen@xxxxxxxxxxxxxxx; hpa@xxxxxxxxx;
>> kirill.shutemov@xxxxxxxxxxxxxxx; richard@xxxxxx;
viro@xxxxxxxxxxxxxxxxxx;
>> sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx; jpoimboe@xxxxxxxxxx;
Elliott,
>> Robert (Servers) <elliott@xxxxxxx>; x86@xxxxxxxxxx;
jussi.kivilinna@xxxxxx
>> Cc: ap420073@xxxxxxxxx
>> 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
