Skip to content

Commit 171f697

Browse files
mlindgrenmlindgren
committed
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
1 parent 60abd9e commit 171f697

File tree

3 files changed

+31
-29
lines changed

3 files changed

+31
-29
lines changed

‎lib/gcm.c‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -351,16 +351,16 @@ SymCryptGcmSetNonce(
351351
// If len(nonce) != 96 bits (12 bytes),
352352
// pre-counter block = GHASH(nonce padded to a multiple of 128 bits || (QWORD) len(nonce))
353353
BYTE buf[SYMCRYPT_GF128_BLOCK_SIZE];
354-
SIZE_T cbNonceRemainder = cbNonce & 0xf;
355-
356-
if(cbNonce >= SYMCRYPT_GF128_BLOCK_SIZE)
357-
{
358-
SymCryptGHashAppendData( &pState->pKey->ghashKey, &pState->ghashState, pbNonce,
359-
cbNonce - cbNonceRemainder );
360-
}
354+
SIZE_T cbNonceRemainder = cbNonce & (SYMCRYPT_GF128_BLOCK_SIZE - 1);
355+
356+
// Process all full blocks of the nonce, i.e. all nonce bytes up to a multiple of
357+
// SYMCRYPT_GF128_BLOCK_SIZE. SymCryptGHashAppendData ignores additional data that are
358+
// not a multiple of the block size. We will handle any such remaining data below.
359+
// (This also works if the nonce is less than the block size.)
360+
SymCryptGHashAppendData( &pState->pKey->ghashKey, &pState->ghashState, pbNonce, cbNonce );
361361

362-
// If the nonce length is not a multiple of 128 bits, it needs to be padded with zeros
363-
// until it is, as GHASH is only defined on multiples of 128 bits.
362+
// If the nonce length is not a multiple of SYMCRYPT_GF128_BLOCK_SIZE, we need to pad any
363+
// remaining data to a multiple of the block size.
364364
if(cbNonceRemainder > 0)
365365
{
366366
SymCryptWipeKnownSize( buf, sizeof(buf) );

‎lib/ghash.c‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,11 @@ SymCryptGHashExpandKeyC(
5959
VOID
6060
SYMCRYPT_CALL
6161
SymCryptGHashAppendDataC(
62-
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
63-
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
64-
_In_reads_( cbData ) PCBYTE pbData,
65-
_In_range_( SYMCRYPT_GF128_BLOCK_SIZE, SIZE_T_MAX & ~0xf) SIZE_T cbData )
62+
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
63+
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
64+
_In_reads_( cbData ) PCBYTE pbData,
65+
SIZE_T cbData )
6666
{
67-
SYMCRYPT_ASSERT(cbData >= SYMCRYPT_GF128_BLOCK_SIZE);
68-
SYMCRYPT_ASSERT((cbData & 0xf) == 0);
69-
7067
UINT64 R0, R1;
7168
UINT64 mask;
7269
SYMCRYPT_ALIGN UINT32 state32[4];
@@ -145,7 +142,7 @@ SymCryptGHashAppendDataXmm(
145142
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
146143
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
147144
_In_reads_( cbData ) PCBYTE pbData,
148-
_In_ SIZE_T cbData )
145+
SIZE_T cbData )
149146
{
150147
__m128i R;
151148
__m128i cmpValue;
@@ -244,7 +241,7 @@ SymCryptGHashAppendDataNeon(
244241
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
245242
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
246243
_In_reads_( cbData ) PCBYTE pbData,
247-
_In_ SIZE_T cbData )
244+
SIZE_T cbData )
248245
{
249246
// Room for improvement: replace non-crypto NEON code below, based on a bit by bit lookup with
250247
// pmull on 8b elements - 8x(8bx8b) -> 8x(16b) pmull is NEON instruction since Armv7
@@ -576,7 +573,7 @@ SymCryptGHashAppendDataPclmulqdq(
576573
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
577574
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
578575
_In_reads_( cbData ) PCBYTE pbData,
579-
_In_ SIZE_T cbData )
576+
SIZE_T cbData )
580577
{
581578
__m128i state;
582579
__m128i data;
@@ -710,7 +707,7 @@ SymCryptGHashAppendDataPmull(
710707
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
711708
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
712709
_In_reads_( cbData ) PCBYTE pbData,
713-
_In_ SIZE_T cbData )
710+
SIZE_T cbData )
714711
{
715712
__n128 state;
716713
__n128 data, datax;
@@ -844,10 +841,10 @@ SymCryptGHashExpandKey(
844841
VOID
845842
SYMCRYPT_CALL
846843
SymCryptGHashAppendData(
847-
_In_ PCSYMCRYPT_GHASH_EXPANDED_KEY expandedKey,
848-
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
849-
_In_reads_( cbData ) PCBYTE pbData,
850-
_In_ SIZE_T cbData )
844+
_In_ PCSYMCRYPT_GHASH_EXPANDED_KEY expandedKey,
845+
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
846+
_In_reads_( cbData ) PCBYTE pbData,
847+
SIZE_T cbData )
851848
{
852849
#if SYMCRYPT_CPU_X86
853850
PCSYMCRYPT_GF128_ELEMENT pExpandedKeyTable;

‎lib/sc_lib.h‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -933,45 +933,50 @@ SymCryptGHashExpandKeyAmd64(
933933
_Out_writes_( SYMCRYPT_GF128_FIELD_SIZE ) PSYMCRYPT_GF128_ELEMENT expandedKey,
934934
_In_reads_( SYMCRYPT_GF128_BLOCK_SIZE ) PCBYTE pH );
935935

936+
//
937+
// For all GHashAppendData functions, data will be appended in multiples of SYMCRYPT_GF128_BLOCK_SIZE.
938+
// If the data is not a multiple of SYMCRYPT_GF128_BLOCK_SIZE, any remaining data will be ignored.
939+
//
940+
936941
VOID
937942
SYMCRYPT_CALL
938943
SymCryptGHashAppendData(
939944
_In_ PCSYMCRYPT_GHASH_EXPANDED_KEY expandedKey,
940945
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
941946
_In_reads_( cbData ) PCBYTE pbData,
942-
_In_ SIZE_T cbData );
947+
SIZE_T cbData );
943948

944949
VOID
945950
SYMCRYPT_CALL
946951
SymCryptGHashAppendDataC(
947952
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
948953
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
949954
_In_reads_( cbData ) PCBYTE pbData,
950-
_In_ SIZE_T cbData );
955+
SIZE_T cbData );
951956

952957
VOID
953958
SYMCRYPT_CALL
954959
SymCryptGHashAppendDataXmm(
955960
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
956961
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
957962
_In_reads_( cbData ) PCBYTE pbData,
958-
_In_ SIZE_T cbData );
963+
SIZE_T cbData );
959964

960965
VOID
961966
SYMCRYPT_CALL
962967
SymCryptGHashAppendDataNeon(
963968
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
964969
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
965970
_In_reads_( cbData ) PCBYTE pbData,
966-
_In_ SIZE_T cbData );
971+
SIZE_T cbData );
967972

968973
VOID
969974
SYMCRYPT_CALL
970975
SymCryptGHashAppendDataPclmulqdq(
971976
_In_reads_( SYMCRYPT_GF128_FIELD_SIZE ) PCSYMCRYPT_GF128_ELEMENT expandedKeyTable,
972977
_Inout_ PSYMCRYPT_GF128_ELEMENT pState,
973978
_In_reads_( cbData ) PCBYTE pbData,
974-
_In_ SIZE_T cbData );
979+
SIZE_T cbData );
975980

976981
VOID
977982
SYMCRYPT_CALL

0 commit comments

Comments
 (0)