Fix SHA3 first-use dispatch race - #2536
Open
Aryadeepta wants to merge 4 commits into
Open
Conversation
Keccak_Dispatch() in xkcp_sha3.c overwrote the global sha3_default_callbacks struct field-by-field on first SHA3 call when AVX512VL was available. Two problems: 1. The struct copy is non-atomic and races against concurrent SHA3 callers on other threads, who can observe a partially-overwritten vtable. 2. The very call that triggered dispatch had already entered through the xkcp _inc_init. After the swap, subsequent _inc_absorb / _inc_finalize on that same context route through the newly-installed AVX512VL table, so one in-flight context is initialized by one backend and absorbed/ finalized by another. Move the top-level backend selection out of Keccak_Dispatch and into a one-time init in sha3.c / sha3x4.c that swaps the 'callbacks' POINTER (single aligned-pointer store) via pthread_once. Keccak_Dispatch still sets the xkcp-internal function pointers (Keccak_*_ptr) for its own use, but no longer touches the top-level table. Signed-off-by: Douglas Stebila <dstebila@uwaterloo.ca>
sha3_avx512vl_callbacks and sha3_x4_avx512vl_callbacks are declared const; storing them in a non-const callbacks pointer required a const-stripping cast that fails under -Werror -Wcast-qual in the fuzzing build. Store the dispatch pointer as const internally; the public API signature is unchanged (non-const input is fine to assign into a const-pointer slot). Try to fix https://github.com/open-quantum-safe/liboqs/actions/runs/25812670070/job/75833444016?pr=2426 Signed-off-by: Douglas Stebila <dstebila@uwaterloo.ca>
Signed-off-by: Douglas Stebila <dstebila@uwaterloo.ca>
Signed-off-by: aryadeepta <aryadeeptade@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2482
Summary
On current
main, the first SHA3/SHAKE call can enter the XKCP callback tablewhile
Keccak_Dispatch()installs the AVX512VL table by copying over the livedefault callback struct. This creates a production data race for concurrent
first use. It also allows one incremental context to be initialized through
XKCP and then absorbed, finalized, or squeezed through AVX512VL.
PR #2426 (Douglas Stebila) previously identified both mechanisms and proposed
moving top-level backend selection into
sha3.c/sha3x4.c. This change appliesthat SHA3 dispatch design to current
main; the reproduction and validationbelow are the additional evidence for issue #2482.
The top-level callback table is now selected once, before public dispatch, with
pthread_oncewhen pthread support is enabled and the existing first-use pathfor non-pthread builds. The selected table is held through a callback-table
pointer.
Keccak_Dispatch()andKeccak_X4_Dispatch()only select XKCP'sinternal implementation pointers.
OQS_SHA3_set_callbacks()andOQS_SHA3_x4_set_callbacks()still accept a custom table after automaticinitialization; concurrent calls to those setter APIs are not made safe by this
change.
Deterministic actual-entry reproduction
Temporary diagnostic instrumentation of the actual XKCP function bodies and
linker wrappers around the AVX512VL assembly entry points was run in a fresh,
single-threaded process. On current
main:With this change:
The trace observes function entry rather than inferring backend identity from a
callback-table read. The temporary instrumentation and wrappers are not part
of this change.
ThreadSanitizer evidence
A temporary reproducer using only the public scalar SHAKE incremental API
(
OQS_SHA3_shake256_inc_*) reports a race betweenKeccak_Dispatch()writingsha3_default_callbacksand another thread reading it fromOQS_SHA3_shake256_inc_initon currentmain. An equivalent public APISHAKE128 x4 workload reports the same race in
Keccak_X4_Dispatch()againstsha3_x4_default_callbacks.The same two workloads report no TSan warnings with this change. The
reproducers do not inspect callback tables or include private SHA3 headers.
Tests
The existing
tests/test_sha3.ccallback override checks were run for scalarand x4 callbacks, along with the SHA3/SHAKE known-answer tests for:
The same test now snapshots representative scalar and x4 first-use callback
entries before first-use incremental dispatch and verifies that those entries
remain unchanged. With the current-main source and this test, the test
fails deterministically with:
With this change it passes.
Commands used for the ordinary test builds included:
The tests passed in AVX512VL distribution, debug, and non-pthread embedded-style
configurations. No SHA3 output, KAT value, public API, or algorithm availability
changed.
The permanent regression test checks representative callback entries rather
than backend identity or every field of each table. The public API intentionally
does not expose the selected backend, so linker wrapping, private symbols, or
production-only tracing would be unnecessarily brittle in the normal suite.
The actual-entry and TSan harnesses are retained as investigation evidence
rather than committed test machinery.
Scope
This work reproduces and fixes a real C data race/undefined concurrent behavior
and a deterministic cross-backend incremental-context ownership violation. No
incorrect digest, production crash, memory corruption, secret leakage, or
security compromise was reproduced, and none is claimed here.
Generative-AI disclosure
I used Codex/ChatGPT to assist with repository investigation, reproducer
development, testing strategy, and drafting. I manually reviewed the resulting
changes and validation results before submission.