Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix undefined behaviour in keccak4x #919

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

jschanck
Copy link
Contributor

There are two uint64_t arrays declared in KeccakP-1600-times4-SIMD256.c that are accessed using _mm256_load_si256 but are not guaranteed to have 32 byte alignment.

#define CONST256(a) _mm256_load_si256((const V256 *) &(a))
[...]
#define ROL64in256_8(d, a) d = _mm256_shuffle_epi8(a, CONST256(rho8))
#define ROL64in256_56(d, a) d = _mm256_shuffle_epi8(a, CONST256(rho56))
static const UINT64 rho8[4] = {0x0605040302010007, 0x0E0D0C0B0A09080F, 0x1615141312111017, 0x1E1D1C1B1A19181F};
static const UINT64 rho56[4] = {0x0007060504030201, 0x080F0E0D0C0B0A09, 0x1017161514131211, 0x181F1E1D1C1B1A19};

This is (likely) the undefined behaviour detected by UBSan in #791.

The first commit of this PR fixes the alignment with a union type. The second commit backports some minor changes to the definitions of other macros in KeccakP-1600-times4-SIMD256.c from XKCP. Only the first commit is related to the undefined behaviour.

The UB issue is also present in XKCP, PQClean, and in our src/kem/kyber/pqcrystals-kyber_common_avx2. However it only manifests when the rho arrays are misaligned at compile time.

With the compilers I've tested, the arrays are correctly aligned when KeccakP-1600-times4-SIMD256.c is its own compilation unit. That's why, as Basil mentioned in the comments of #791, UBSan does not flag the new Kyber code.

We just happened to get unlucky with the layout of data sections when we included KeccakP-1600-times4-SIMD256.c into src/common/sha3/sha3x4_avx2.c. The situation could reverse with a compiler update, etc, so we should leave #791 open until it's fixed in Kyber or we switch Kyber over to common keccak4x.

@dstebila dstebila merged commit 952c628 into open-quantum-safe:main Feb 18, 2021
@jschanck jschanck deleted the ub.keccak4x branch April 8, 2021 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants