Tianjia Zhang tianjia.zhang@linux.alibaba.com writes:
Signed-off-by: Tianjia Zhang tianjia.zhang@linux.alibaba.com
Makefile.in | 1 + nettle-meta-ciphers.c | 1 + nettle-meta.h | 2 + sm4-meta.c | 49 ++++++++ sm4.c | 225 +++++++++++++++++++++++++++++++++++ sm4.h | 71 +++++++++++ testsuite/meta-cipher-test.c | 3 +- 7 files changed, 351 insertions(+), 1 deletion(-) create mode 100644 sm4-meta.c create mode 100644 sm4.c create mode 100644 sm4.h
Overall looks pretty good. But I wonder if one could avoid having two copies of the subkeys.
+void +sm4_set_key(struct sm4_ctx *ctx, const uint8_t *key) +{
- uint32_t rk[4];
- int i;
- rk[0] = READ_UINT32(key + 0) ^ fk[0];
- rk[1] = READ_UINT32(key + 4) ^ fk[1];
- rk[2] = READ_UINT32(key + 8) ^ fk[2];
- rk[3] = READ_UINT32(key + 12) ^ fk[3];
- for (i = 0; i < 32; i += 4)
- {
rk[0] ^= sm4_key_sub(rk[1] ^ rk[2] ^ rk[3] ^ ck[i + 0]);rk[1] ^= sm4_key_sub(rk[2] ^ rk[3] ^ rk[0] ^ ck[i + 1]);rk[2] ^= sm4_key_sub(rk[3] ^ rk[0] ^ rk[1] ^ ck[i + 2]);rk[3] ^= sm4_key_sub(rk[0] ^ rk[1] ^ rk[2] ^ ck[i + 3]);ctx->rkey_enc[i + 0] = rk[0];ctx->rkey_enc[i + 1] = rk[1];ctx->rkey_enc[i + 2] = rk[2];ctx->rkey_enc[i + 3] = rk[3];ctx->rkey_dec[31 - 0 - i] = rk[0];ctx->rkey_dec[31 - 1 - i] = rk[1];ctx->rkey_dec[31 - 2 - i] = rk[2];ctx->rkey_dec[31 - 3 - i] = rk[3];- }
+}
So subkeys are identical for encrypt and decrypt, just used in opposite order? It seems unnecessary to use two copies.
+static void +sm4_crypt_block(const uint32_t *rk, uint8_t *dst, const uint8_t *src) +{
- uint32_t x[4], i;
The loop counter i should have type plain int (or unsigned; in Nettle I tend to use unsigned for non-negative values).
- x[0] = READ_UINT32(src + 0 * 4);
- x[1] = READ_UINT32(src + 1 * 4);
- x[2] = READ_UINT32(src + 2 * 4);
- x[3] = READ_UINT32(src + 3 * 4);
- for (i = 0; i < 32; i += 4)
- {
x[0] = sm4_round(x[0], x[1], x[2], x[3], rk[i + 0]);x[1] = sm4_round(x[1], x[2], x[3], x[0], rk[i + 1]);x[2] = sm4_round(x[2], x[3], x[0], x[1], rk[i + 2]);x[3] = sm4_round(x[3], x[0], x[1], x[2], rk[i + 3]);- }
Since the x[] array is indexed only by constants, you could consider using scalar variables
uint32_t x0, x1, x2 x3;
Probably makes no difference with modern compilers, but we'd like the values to be allocated in registers, not as an array on the stack. And similarly for the rk array above.
- WRITE_UINT32(dst + 0 * 4, x[3 - 0]);
- WRITE_UINT32(dst + 1 * 4, x[3 - 1]);
- WRITE_UINT32(dst + 2 * 4, x[3 - 2]);
- WRITE_UINT32(dst + 3 * 4, x[3 - 3]);
+}
If this is the same for encrypt and decrypt, you could move the block loop into this function too, to avoid code duplication.
+void +sm4_encrypt(const struct sm4_ctx *context,
size_t length,uint8_t *dst,const uint8_t *src)+{
- const uint32_t *keys = context->rkey_enc;
[...]
+void +sm4_decrypt(const struct sm4_ctx *context,
size_t length,uint8_t *dst,const uint8_t *src)+{
- const uint32_t *keys = context->rkey_dec;
I see two alternatives to use only one copy of the subkeys:
1. Keep using a single sm4_set_key function, but store only the first copy (currently rkey_enc). Change the sm4_decrypt function to access subkeys in the opposite order. To keep a shared sm4_crypt_block function, that function would need another +1/-1 argument used to determine subkey access order.
2. Implement two separate functions sm4_set_encrypt_key and sm4_set_decrypt_key, where the latter stores the subkeys in opposite order. sm4_set_decrypt_key could be implemented as sm4_set_encrypt_key + sm4_invert_key, where sm4_invert_key is a function that just reverses the order (a bit similar to _nettle_aes_invert, but simpler). Then the same sm4_crypt function can be used for both encrypt and decrypt.
Not sure what's best, but I'd lean towards (2), since simplicity of the more performance critical encrypt/decrypt operation seems more important than a simplicity at key setup.
Regards, /Niels