Re: [RFC PATCH v3 1/2] fscrypt-crypt-util: add HCTR2 reference implementation

[Eric Biggers <ebiggers@kernel.org>]

Please include the linux-fscrypt mailing list in Cc.

On Fri, May 20, 2022 at 01:23:14PM -0500, Nathan Huckleberry wrote:
> This patch adds a reference implementation of HCTR2 to the fscrypt
> testing utility.

The reference implementation is at https://github.com/google/hctr2.
This one is just an "implementation", not a "reference implementation".

>  /* Multiply a GF(2^128) element by the polynomial 'x' */
> -static inline void gf2_128_mul_x(ble128 *t)
> +static inline void gf2_128_mul_x_ble(ble128 *t)
>  {
>  	u64 lo = le64_to_cpu(t->lo);
>  	u64 hi = le64_to_cpu(t->hi);
> @@ -302,6 +356,38 @@ static inline void gf2_128_mul_x(ble128 *t)
>  	t->lo = cpu_to_le64((lo << 1) ^ ((hi & (1ULL << 63)) ? 0x87 : 0));
>  }
>  
> +static inline void gf2_128_mul_x_polyval(ble128 *t)
> +{

Please adjust the naming and comments to clearly distinguish between
gf2_128_mul_x_ble() and gf2_128_mul_x_polyval().  The intention of the original
code was that "ble" gave the field convention used.  However, the XTS and
POLYVAL field conventions both use "ble" but differ in the reduction polynomial.
So "ble" is no longer unambiguous.  How about renaming gf2_128_mul_x_ble() to
gf2_128_mul_x_xts() to indicate it is using the XTS convention?

> +	u64 lo = le64_to_cpu(t->lo);
> +	u64 hi = le64_to_cpu(t->hi);
> +
> +	u64 lo_reducer = ((hi >> 63) & 1ULL) ? 1 : 0;
> +	u64 hi_reducer = ((hi >> 63) & 1ULL) ? 0xc2ULL << 56 : 0;

There shouldn't be a newline between variable declarations.

Also maybe write '(hi & (1ULL << 63))', so the code is more easily comparable to
gf2_128_mul_x_xts().

> +void gf2_128_mul_polyval(ble128 *r, const ble128 *b)
> +{
> +	ble128 p;
> +	u64 lo = le64_to_cpu(b->lo);
> +	u64 hi = le64_to_cpu(b->hi);
> +
> +	memset(&p, 0, sizeof(p));
> +	for (int i = 0; i < 64; i++) {

'i' should be declared at the beginning.

> +		if (lo & (1ULL << i))
> +			xor((u8 *)&p, (u8 *)&p, (u8 *)r, sizeof(p));
> +		gf2_128_mul_x_polyval(r);
> +	}
> +	for (int i = 0; i < 64; i++) {
> +		if (hi & (1ULL << i))
> +			xor((u8 *)&p, (u8 *)&p, (u8 *)r, sizeof(p));
> +		gf2_128_mul_x_polyval(r);
> +	}
> +	memcpy(r, &p, sizeof(p));
> +}

The last line can be '*r = p;'

> +
>  /*----------------------------------------------------------------------------*
>   *                             Group arithmetic                               *
>   *----------------------------------------------------------------------------*/
> @@ -901,6 +987,41 @@ static void test_hkdf_sha512(void)
>  }
>  #endif /* ENABLE_ALG_TESTS */
>  
> +/*----------------------------------------------------------------------------*
> + *                                 POLYVAL                                     *
> + *----------------------------------------------------------------------------*/
> +
> +#define POLYVAL_KEY_SIZE	16
> +#define POLYVAL_BLOCK_SIZE	16
> +
> +static void polyval_update(const u8 key[POLYVAL_KEY_SIZE],
> +			   const u8 *msg, size_t msglen,
> +			   u8 accumulator[POLYVAL_BLOCK_SIZE])
> +{
> +	ble128 h;
> +	// x^{-128} = x^127 + x^124 + x^121 + x^114 + 1
> +	static const ble128 inv128 = {
> +	    cpu_to_le64(1),
> +	    cpu_to_le64(0x9204ULL << 48)
> +	};

Use tabs for indentation, please.  This applies elsewhere in the file too.

> +	int nblocks = msglen / POLYVAL_BLOCK_SIZE;

size_t

> +	int tail = msglen % POLYVAL_BLOCK_SIZE;
> +
> +	memcpy(&h, key, sizeof(h));
> +	gf2_128_mul_polyval(&h, &inv128);
> +
> +	while (nblocks > 0) {
> +		xor(accumulator, accumulator, msg, POLYVAL_BLOCK_SIZE);
> +		gf2_128_mul_polyval((ble128 *)accumulator, &h);

Casting a byte array to ble128, and then dereferencing it, is undefined
behavior since it increases the alignment that is assumed.

> +#define HCTR2_IV_SIZE 32
> +static void aes_256_hctr2_crypt(const u8 key[AES_256_KEY_SIZE],
> +				const u8 iv[HCTR2_IV_SIZE], const u8 *src,
> +				u8 *dst, size_t nbytes, bool decrypting)
> +{
> +	struct aes_key k;
> +	u8 hbar[AES_BLOCK_SIZE] = {0};
> +	u8 L[AES_BLOCK_SIZE] = {1};
> +	size_t bulk_bytes = nbytes - AES_BLOCK_SIZE;
> +	size_t remainder = bulk_bytes % AES_BLOCK_SIZE;
> +	le128 tweaklen_blk = {
> +	    .lo = cpu_to_le64(HCTR2_IV_SIZE * 8 * 2 + 2 + (remainder != 0))
> +	};
> +	u8 padded_block[POLYVAL_BLOCK_SIZE];
> +	u8 digest[POLYVAL_BLOCK_SIZE];
> +	const u8 *M = src;
> +	const u8 *N = src + AES_BLOCK_SIZE;
> +	u8 MM[AES_BLOCK_SIZE];
> +	u8 UU[AES_BLOCK_SIZE];
> +	u8 S[AES_BLOCK_SIZE];
> +	u8 *U = dst;
> +	u8 *V = dst + AES_BLOCK_SIZE;
> +
> +	ASSERT(nbytes >= AES_BLOCK_SIZE);
> +	aes_setkey(&k, key, AES_256_KEY_SIZE);
> +
> +	aes_encrypt(&k, hbar, hbar);
> +	aes_encrypt(&k, L, L);
> +
> +	memset(digest, 0, POLYVAL_BLOCK_SIZE);
> +	polyval_update(hbar, (u8 *)&tweaklen_blk, POLYVAL_BLOCK_SIZE, digest);
> +	polyval_update(hbar, iv, HCTR2_IV_SIZE, digest);
> +
> +	polyval_update(hbar, N, bulk_bytes - remainder, digest);
> +	if (remainder) {
> +		memset(padded_block, 0, POLYVAL_BLOCK_SIZE);
> +		memcpy(padded_block, N + bulk_bytes - remainder, remainder);
> +		padded_block[remainder] = 0x01;
> +		polyval_update(hbar, padded_block, POLYVAL_BLOCK_SIZE, digest);
> +	}
> +
> +	xor(MM, M, digest, AES_BLOCK_SIZE);
> +
> +	if (decrypting)
> +		aes_decrypt(&k, MM, UU);
> +	else
> +		aes_encrypt(&k, MM, UU);
> +
> +	xor(S, MM, UU, AES_BLOCK_SIZE);
> +	xor(S, L, S, AES_BLOCK_SIZE);
> +
> +	aes_256_xctr_crypt(key, S, N, V, bulk_bytes);
> +
> +	memset(digest, 0, POLYVAL_BLOCK_SIZE);
> +	polyval_update(hbar, (u8 *)&tweaklen_blk, POLYVAL_BLOCK_SIZE, digest);
> +	polyval_update(hbar, iv, HCTR2_IV_SIZE, digest);
> +
> +	polyval_update(hbar, V, bulk_bytes - remainder, digest);
> +	if (remainder) {
> +		memset(padded_block, 0, POLYVAL_BLOCK_SIZE);
> +		memcpy(padded_block, V + bulk_bytes - remainder, remainder);
> +		padded_block[remainder] = 0x01;
> +		polyval_update(hbar, padded_block, POLYVAL_BLOCK_SIZE, digest);
> +	}
> +
> +	xor(U, UU, digest, AES_BLOCK_SIZE);
> +}

Long functions with many local variables are hard to read.  This would be easier
to read if it used some helper functions, each called twice:

static void hctr2_hash_iv(const u8 hbar[POLYVAL_KEY_SIZE],
			  const u8 iv[HCTR2_IV_SIZE], size_t msglen,
			  u8 digest[POLYVAL_BLOCK_SIZE])
{
	le128 tweaklen_blk = {
		.lo = cpu_to_le64(HCTR2_IV_SIZE * 8 * 2 + 2 +
				  (msglen % AES_BLOCK_SIZE != 0))
	};

	memset(digest, 0, POLYVAL_BLOCK_SIZE);
	polyval_update(hbar, (u8 *)&tweaklen_blk, POLYVAL_BLOCK_SIZE, digest);
	polyval_update(hbar, iv, HCTR2_IV_SIZE, digest);
}

static void hctr2_hash_message(const u8 hbar[POLYVAL_KEY_SIZE],
			       const u8 *msg, size_t msglen,
			       u8 digest[POLYVAL_BLOCK_SIZE])
{
	size_t remainder = msglen % AES_BLOCK_SIZE;
	u8 padded_block[POLYVAL_BLOCK_SIZE] = {0};

	polyval_update(hbar, msg, msglen - remainder, digest);
	if (remainder) {
		memcpy(padded_block, &msg[msglen - remainder], remainder);
		padded_block[remainder] = 1;
		polyval_update(hbar, padded_block, POLYVAL_BLOCK_SIZE, digest);
	}
}

> +		rand_bytes(key, sizeof(key));
> +		rand_bytes(iv, sizeof(iv));
> +		rand_bytes(ptext, datalen);
> +		memset(key, 0, sizeof(key));
> +		memset(iv, 0, sizeof(iv));
> +		memset(ptext, 0, datalen);

What's going on here?  The memset() calls shouldn't be there.

- Eric