From 171f6973dab9b76f0dc61d966d3e977021325bc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mitch=20Lindgren=20=F0=9F=A6=8E?= Date: Fri, 12 Apr 2024 05:54:16 +0000 Subject: [PATCH] Merged PR 10593466: Clean up GHash assertions and SAL annotations based on feedback Another follow up to !10578579, this PR removes unnecessary assertions and SAL annotations from `GHashAppendData*` to be more consistent with other SymCrypt functions. It turns out that passing in data that aren't a multiple of the block size can sometimes be convenient because it allows one to make calls to the function unconditional. I added a comment at the function definition to indicate that data beyond multiples of the block size are ignored. Tested: local unit tests (AMD64 noasm), CI pipelines --- lib/gcm.c | 18 +++++++++--------- lib/ghash.c | 27 ++++++++++++--------------- lib/sc_lib.h | 15 ++++++++++----- 3 files changed, 31 insertions(+), 29 deletions(-) diff --git a/lib/gcm.c b/lib/gcm.c index de9c5490..294267df 100644 --- a/lib/gcm.c +++ b/lib/gcm.c @@ -351,16 +351,16 @@ SymCryptGcmSetNonce( // If len(nonce) != 96 bits (12 bytes), // pre-counter block = GHASH(nonce padded to a multiple of 128 bits || (QWORD) len(nonce)) BYTE buf[SYMCRYPT_GF128_BLOCK_SIZE]; - SIZE_T cbNonceRemainder = cbNonce & 0xf; - - if(cbNonce >= SYMCRYPT_GF128_BLOCK_SIZE) - { - SymCryptGHashAppendData( &pState->pKey->ghashKey, &pState->ghashState, pbNonce, - cbNonce - cbNonceRemainder ); - } + SIZE_T cbNonceRemainder = cbNonce & (SYMCRYPT_GF128_BLOCK_SIZE - 1); + + // Process all full blocks of the nonce, i.e. all nonce bytes up to a multiple of + // SYMCRYPT_GF128_BLOCK_SIZE. SymCryptGHashAppendData ignores additional data that are + // not a multiple of the block size. We will handle any such remaining data below. + // (This also works if the nonce is less than the block size.) + SymCryptGHashAppendData( &pState->pKey->ghashKey, &pState->ghashState, pbNonce, cbNonce ); - // If the nonce length is not a multiple of 128 bits, it needs to be padded with zeros - // until it is, as GHASH is only defined on multiples of 128 bits. + // If the nonce length is not a multiple of SYMCRYPT_GF128_BLOCK_SIZE, we need to pad any + // remaining data to a multiple of the block size. if(cbNonceRemainder > 0) { SymCryptWipeKnownSize( buf, sizeof(buf) ); diff --git a/lib/ghash.c b/lib/ghash.c index 463e443b..9d1c1fe7 100644 --- a/lib/ghash.c +++ b/lib/ghash.c @@ -59,14 +59,11 @@ SymCryptGHashExpandKeyC( VOID SYMCRYPT_CALL SymCryptGHashAppendDataC( - _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, - _Inout_ PSYMCRYPT_GF128_ELEMENT pState, - _In_reads_( cbData ) PCBYTE pbData, - _In_range_( SYMCRYPT_GF128_BLOCK_SIZE, SIZE_T_MAX & ~0xf) SIZE_T cbData ) + _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, + _Inout_ PSYMCRYPT_GF128_ELEMENT pState, + _In_reads_( cbData ) PCBYTE pbData, + SIZE_T cbData ) { - SYMCRYPT_ASSERT(cbData >= SYMCRYPT_GF128_BLOCK_SIZE); - SYMCRYPT_ASSERT((cbData & 0xf) == 0); - UINT64 R0, R1; UINT64 mask; SYMCRYPT_ALIGN UINT32 state32[4]; @@ -145,7 +142,7 @@ SymCryptGHashAppendDataXmm( _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ) + SIZE_T cbData ) { __m128i R; __m128i cmpValue; @@ -244,7 +241,7 @@ SymCryptGHashAppendDataNeon( _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ) + SIZE_T cbData ) { // Room for improvement: replace non-crypto NEON code below, based on a bit by bit lookup with // pmull on 8b elements - 8x(8bx8b) -> 8x(16b) pmull is NEON instruction since Armv7 @@ -576,7 +573,7 @@ SymCryptGHashAppendDataPclmulqdq( _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ) + SIZE_T cbData ) { __m128i state; __m128i data; @@ -710,7 +707,7 @@ SymCryptGHashAppendDataPmull( _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ) + SIZE_T cbData ) { __n128 state; __n128 data, datax; @@ -844,10 +841,10 @@ SymCryptGHashExpandKey( VOID SYMCRYPT_CALL SymCryptGHashAppendData( - _In_ PCSYMCRYPT_GHASH_EXPANDED_KEY expandedKey, - _Inout_ PSYMCRYPT_GF128_ELEMENT pState, - _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ) + _In_ PCSYMCRYPT_GHASH_EXPANDED_KEY expandedKey, + _Inout_ PSYMCRYPT_GF128_ELEMENT pState, + _In_reads_( cbData ) PCBYTE pbData, + SIZE_T cbData ) { #if SYMCRYPT_CPU_X86 PCSYMCRYPT_GF128_ELEMENT pExpandedKeyTable; diff --git a/lib/sc_lib.h b/lib/sc_lib.h index 10a09911..b614179e 100644 --- a/lib/sc_lib.h +++ b/lib/sc_lib.h @@ -933,13 +933,18 @@ SymCryptGHashExpandKeyAmd64( _Out_writes_( SYMCRYPT_GF128_FIELD_SIZE ) PSYMCRYPT_GF128_ELEMENT expandedKey, _In_reads_( SYMCRYPT_GF128_BLOCK_SIZE ) PCBYTE pH ); +// +// For all GHashAppendData functions, data will be appended in multiples of SYMCRYPT_GF128_BLOCK_SIZE. +// If the data is not a multiple of SYMCRYPT_GF128_BLOCK_SIZE, any remaining data will be ignored. +// + VOID SYMCRYPT_CALL SymCryptGHashAppendData( _In_ PCSYMCRYPT_GHASH_EXPANDED_KEY expandedKey, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ); + SIZE_T cbData ); VOID SYMCRYPT_CALL @@ -947,7 +952,7 @@ SymCryptGHashAppendDataC( _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ); + SIZE_T cbData ); VOID SYMCRYPT_CALL @@ -955,7 +960,7 @@ SymCryptGHashAppendDataXmm( _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ); + SIZE_T cbData ); VOID SYMCRYPT_CALL @@ -963,7 +968,7 @@ SymCryptGHashAppendDataNeon( _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ); + SIZE_T cbData ); VOID SYMCRYPT_CALL @@ -971,7 +976,7 @@ SymCryptGHashAppendDataPclmulqdq( _In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable, _Inout_ PSYMCRYPT_GF128_ELEMENT pState, _In_reads_( cbData ) PCBYTE pbData, - _In_ SIZE_T cbData ); + SIZE_T cbData ); VOID SYMCRYPT_CALL