[PATCH] selftests: vDSO: Also test counter in vdso_test_chacha

[PATCH] selftests: vDSO: Also test counter in vdso_test_chacha

[Christophe Leroy]

The chacha vDSO selftest doesn't check the way the counter is handled
by __arch_chacha20_blocks_nostack(). It indirectly checks that the
counter is writen on exit and read back on new entry, but it doesn't
check that the format is correct. It has led to an invisible erroneous
implementation on powerpc where the counter was writen and read in
wrong byte order.

Also, the counter uses two words, but the tests with a zero counter
and uses a small amount of blocks so at the end the upper part of the
counter is always 0 so it is not checked.

Add a verification of counter's content in addition to the
verification of the output.

Also add two tests where the counter crosses the u32 upper limit. The
first test verifies that the function properly writes back the upper
word, the second test verifies that the function properly reads back
the upper word.

While at it, remove 'nonce' which is not unused anymore after the
replacement of libsodium by open coded chacha implementation.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 .../testing/selftests/vDSO/vdso_test_chacha.c | 39 ++++++++++++++-----
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha.c b/tools/testing/selftests/vDSO/vdso_test_chacha.c
index 9d18d49a82f8..ed6cf372d9ee 100644
--- a/tools/testing/selftests/vDSO/vdso_test_chacha.c
+++ b/tools/testing/selftests/vDSO/vdso_test_chacha.c
@@ -17,11 +17,12 @@ static uint32_t rol32(uint32_t word, unsigned int shift)
 	return (word << (shift & 31)) | (word >> ((-shift) & 31));
 }
 
-static void reference_chacha20_blocks(uint8_t *dst_bytes, const uint32_t *key, size_t nblocks)
+static void reference_chacha20_blocks(uint8_t *dst_bytes, const uint32_t *key, uint32_t *counter, size_t nblocks)
 {
 	uint32_t s[16] = {
 		0x61707865U, 0x3320646eU, 0x79622d32U, 0x6b206574U,
-		key[0], key[1], key[2], key[3], key[4], key[5], key[6], key[7]
+		key[0], key[1], key[2], key[3], key[4], key[5], key[6], key[7],
+		counter[0], counter[1],
 	};
 
 	while (nblocks--) {
@@ -52,6 +53,8 @@ static void reference_chacha20_blocks(uint8_t *dst_bytes, const uint32_t *key, s
 		if (!++s[12])
 			++s[13];
 	}
+	counter[0] = s[12];
+	counter[1] = s[13];
 }
 
 typedef uint8_t u8;
@@ -66,8 +69,7 @@ typedef uint64_t u64;
 int main(int argc, char *argv[])
 {
 	enum { TRIALS = 1000, BLOCKS = 128, BLOCK_SIZE = 64 };
-	static const uint8_t nonce[8] = { 0 };
-	uint32_t counter[2];
+	uint32_t counter1[2], counter2[2];
 	uint32_t key[8];
 	uint8_t output1[BLOCK_SIZE * BLOCKS], output2[BLOCK_SIZE * BLOCKS];
 
@@ -84,17 +86,36 @@ int main(int argc, char *argv[])
 			printf("getrandom() failed!\n");
 			return KSFT_SKIP;
 		}
-		reference_chacha20_blocks(output1, key, BLOCKS);
+		memset(counter1, 0, sizeof(counter1));
+		reference_chacha20_blocks(output1, key, counter1, BLOCKS);
 		for (unsigned int split = 0; split < BLOCKS; ++split) {
 			memset(output2, 'X', sizeof(output2));
-			memset(counter, 0, sizeof(counter));
+			memset(counter2, 0, sizeof(counter2));
 			if (split)
-				__arch_chacha20_blocks_nostack(output2, key, counter, split);
-			__arch_chacha20_blocks_nostack(output2 + split * BLOCK_SIZE, key, counter, BLOCKS - split);
-			if (memcmp(output1, output2, sizeof(output1)))
+				__arch_chacha20_blocks_nostack(output2, key, counter2, split);
+			__arch_chacha20_blocks_nostack(output2 + split * BLOCK_SIZE, key, counter2, BLOCKS - split);
+			if (memcmp(output1, output2, sizeof(output1)) ||
+			    memcmp(counter2, counter2, sizeof(counter1)))
 				return KSFT_FAIL;
 		}
 	}
+	memset(counter1, 0, sizeof(counter1));
+	counter1[0] = (uint32_t)-BLOCKS + 2;
+	memset(counter2, 0, sizeof(counter2));
+	counter2[0] = (uint32_t)-BLOCKS + 2;
+
+	reference_chacha20_blocks(output1, key, counter1, BLOCKS);
+	__arch_chacha20_blocks_nostack(output2, key, counter2, BLOCKS);
+	if (memcmp(output1, output2, sizeof(output1)) ||
+	    memcmp(counter2, counter2, sizeof(counter1)))
+		return KSFT_FAIL;
+
+	reference_chacha20_blocks(output1, key, counter1, BLOCKS);
+	__arch_chacha20_blocks_nostack(output2, key, counter2, BLOCKS);
+	if (memcmp(output1, output2, sizeof(output1)) ||
+	    memcmp(counter2, counter2, sizeof(counter1)))
+		return KSFT_FAIL;
+
 	ksft_test_result_pass("chacha: PASS\n");
 	return KSFT_PASS;
 }
-- 
2.44.0

Re: [PATCH] selftests: vDSO: Also test counter in vdso_test_chacha

[Jason A. Donenfeld]

On Sun, Sep 01, 2024 at 07:40:33PM +0200, Christophe Leroy wrote:
> -static void reference_chacha20_blocks(uint8_t *dst_bytes, const uint32_t *key, size_t nblocks)
> +static void reference_chacha20_blocks(uint8_t *dst_bytes, const uint32_t *key, uint32_t *counter, size_t nblocks)
>  {
>  	uint32_t s[16] = {
>  		0x61707865U, 0x3320646eU, 0x79622d32U, 0x6b206574U,
> -		key[0], key[1], key[2], key[3], key[4], key[5], key[6], key[7]
> +		key[0], key[1], key[2], key[3], key[4], key[5], key[6], key[7],
> +		counter[0], counter[1],

While you're doing this, also add the remaining, `0, 0` elements.

> +			if (memcmp(output1, output2, sizeof(output1)) ||
> +			    memcmp(counter2, counter2, sizeof(counter1)))

counter2 will always be counter2. You meant for the first argument to be
counter1.

> +	reference_chacha20_blocks(output1, key, counter1, BLOCKS);
> +	__arch_chacha20_blocks_nostack(output2, key, counter2, BLOCKS);
> +	if (memcmp(output1, output2, sizeof(output1)) ||
> +	    memcmp(counter2, counter2, sizeof(counter1)))
> +		return KSFT_FAIL;
> +
> +	reference_chacha20_blocks(output1, key, counter1, BLOCKS);
> +	__arch_chacha20_blocks_nostack(output2, key, counter2, BLOCKS);
> +	if (memcmp(output1, output2, sizeof(output1)) ||
> +	    memcmp(counter2, counter2, sizeof(counter1)))
> +		return KSFT_FAIL;
> +

Why repeat these two stanzas? Also, same issue with counter2 used twice
in that memcmp.

Re: [PATCH] selftests: vDSO: Also test counter in vdso_test_chacha

[Jason A. Donenfeld]

On Sun, Sep 01, 2024 at 08:08:13PM +0200, Jason A. Donenfeld wrote:
> > +	reference_chacha20_blocks(output1, key, counter1, BLOCKS);
> > +	__arch_chacha20_blocks_nostack(output2, key, counter2, BLOCKS);
> > +	if (memcmp(output1, output2, sizeof(output1)) ||
> > +	    memcmp(counter2, counter2, sizeof(counter1)))
> > +		return KSFT_FAIL;
> > +
> > +	reference_chacha20_blocks(output1, key, counter1, BLOCKS);
> > +	__arch_chacha20_blocks_nostack(output2, key, counter2, BLOCKS);
> > +	if (memcmp(output1, output2, sizeof(output1)) ||
> > +	    memcmp(counter2, counter2, sizeof(counter1)))
> > +		return KSFT_FAIL;
> > +
> 
> Why repeat these two stanzas? 

Ah, from your commit message:

"The first test verifies that the function properly writes back the
upper word, the second test verifies that the function properly reads
back the upper word."

Re: [PATCH] selftests: vDSO: Also test counter in vdso_test_chacha

[Jason A. Donenfeld]

On Sun, Sep 01, 2024 at 08:08:13PM +0200, Jason A. Donenfeld wrote:
> While you're doing this, also add the remaining, `0, 0` elements.
>
> counter2 will always be counter2. You meant for the first argument to be
> counter1.
>
> Also, same issue with counter2 used twice in that memcmp.

Fixed these up and applied it.