Update HQC implementation to 2025-08-22 version#2407
Conversation
9fc0a48 to
fc1072b
Compare
|
Analysis by Claude about Windows failure: Root cause is in the BITMASK macro. The bug src/kem/hqc/xuganyu96-hqc_hqc-*_ref/parameters.h:12: On Windows x86_64 (LLP64) unsigned long is 32 bits. On Linux/macOS x86_64 (LP64) it's 64 bits. So 1UL << 37 is fine everywhere except Windows, where it's undefined behavior (shift ≥ operand width) and MSVC typically emits an x86 shl that masks the Why only HQC-5: ┌─────────┬─────────┬──────────────┬─────────────┐ The mask is applied at:
So every vect_mul result and every sampled h has its top 37 bits zeroed on Windows, producing corrupted keys/ciphertexts — exactly what your debug fprintf of s, y, h in hqc_pke_keygen should expose (look at s[VEC_N_SIZE_64 - 1] and h[VEC_N_SIZE_64 - Why "when SLH-DSA is disabled" is a red herring .github/workflows/windows.yml:19,52 unconditionally passes -DOQS_ENABLE_SIG_SLH_DSA=OFF. That's just the Windows CI config — not causal. The bug fires on Windows regardless of SLH-DSA. Fix Change 1UL → 1ULL (or UINT64_C(1)) in all three parameters.h files. Also consider (a) % (size) parens, but the UL→ULL is the actual fix. |
d4fd2d7 to
fc1072b
Compare
37f3f99 to
627be2b
Compare
|
Hi @xuganyu96 ! Thank you for the immense amount of work in order to see this PR through, I greatly appreciate it. I have a general question: The source implementation maintains a certain amount of "common" code (e.g. |
The short reason is simplicity of configuration files and ease of development. I am aware that only recently I have pushed the MQOM contributor to de-duplicate files, but I am currently prioritizing pushing HQC across the finish line so we can release 0.16. I am happy to come back to this. |
487c7ff to
955e000
Compare
That decision seems sensible to me, seeing as we are gearing towards a release soon and Thanks for the work and for seeing this through @xuganyu96 ! |
|
It would be ideal to reach out to the HQC team for an "official" liboqs integration effort. In case that is not feasible, I think it makes sense to move the fork of official repository under |
In my opinion, it would not only be ideal, but necessary -- unless of course you now consider yourself an expert in the HQC code @xuganyu96 : Do you? The discussion above (relying on Claude "knowledge/advice") doesn't quite create that impression, but I may be jumping to conclusions here. Finally, moving a fork into the oqs namespace in my opinion creates the impression and expectation that OQS becomes/is the final "source of truth" and maintainer of last resort of an algorithm. I would think we should do that only if we have absolute certainty we understand the code and logic inside-out and can maintain it going forward under all circumstances (even if single members leave the project). I'm not sure this applies here and suggest having a TSC discussion about this. |
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
On certain platforms, "unsigned long" is 32-bit wide, so (1UL << x) can overflow if x is greater than 31, such as with HQC-5. Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
WHY WHY WHY what is even the difference???? Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
For liboqs integration, hash_g implementation has been modified There are some unexpected dispatch errors under liboqs/src/common/sha3 that causes intermittent GitHub Action test failures. Due to the difficulty of debugging on GitHub action, I chose this shortcut of replacing incremental API with a single hash over a continuous strip of memory. There might be performance and stack memory usage penalty associated with this approach, but I deemed it acceptable. we need to eventually figure out what happened with Keccak dispatch and restore the usage of incremental API Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
`debuglogging` was a convenient debugging tool I added during the development processs. Though it was helpful, it is not in the scope of HQC integration, so it is removed in this branch. I will gauge interest and implement the debuglogging feature separately. Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
9dd1568 to
232fd29
Compare
Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
The regression test was created for an older implementation of HQC. After #2407 merge, HQC would be updated to a newer spec with a different upstream for implementation. HQC would also become enabled by default and tested for constant-time behaviors through Valgrind memcheck. A separate constant-time test for HQC is no longer necessary. Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
The regression test was created for an older implementation of HQC. After #2407 merge, HQC would be updated to a newer spec with a different upstream for implementation. HQC would also become enabled by default and tested for constant-time behaviors through Valgrind memcheck. A separate constant-time test for HQC is no longer necessary. Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
…st (#2467) * [extended tests] Suppress memcheck errors in McEliece Added the following functions for valgrind memcheck to ignore: - to_bitslicing_2x, called by PQCLEAN_MCELIECE<param>_AVX2_pk_gen - vec256_cswap, called by PQCLEAN_MCELIECE<param>_AVX2_pk_gen - PQCLEAN_MCELIECE<param>_AVX2_pk_gen - Everything in vec256_mul_asm.S This is admittedly not ideal: I don't fully understand why these functions were flagged by Valgrind and whether they are safe to ignore. However, considering that PQClean was archived and liboqs will be moving onto other McEliece impl sources, I find it acceptable to "just make the error go away" Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Remove HQC regression test after #2407 merge The regression test was created for an older implementation of HQC. After #2407 merge, HQC would be updated to a newer spec with a different upstream for implementation. HQC would also become enabled by default and tested for constant-time behaviors through Valgrind memcheck. A separate constant-time test for HQC is no longer necessary. Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> --------- Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
|
Thanks for the work in seeing this through, @xuganyu96 ! |
* Skeleton release Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * [skip ci] draft release notes, with to-do's Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Improved writing on release notes Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Format markdown Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Markdown formatting Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Let it rip [extended tests] [full tests] Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Double-checked first-time contributors Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * [skip ci] Updated release notes Included #2460, #2407, #2467, #2463, #2441 Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Significant changes deserve more explanation Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Update release notes after #2431 merge Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Update RELEASE.md after #2470 merge Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> * Update 0.16.0-rc1 release date Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca> --------- Signed-off-by: Ganyu (Bruce) Xu <g66xu@uwaterloo.ca>
This pull request replaces the PQClean implementation of HQC with the reference implementation, originally hosted on GitLab. Fixes #2118
All code change is developed on a personal fork. After reaching sufficient maturity, the code change is git-diff'ed into a patch file. This admittedly will make future development more tedious, so let me know if we should continue using a personal fork. We can also reach out to the team to merge our code change.
Code change
There are several significant changes to the original implementation:
Namespacing was added to the original implementation. Every non-static symbol has a
PQCHQC_NAMESPACEmacro applied so that a user-defined prefix will be attached (e.g.PQCHQC_HQC1_C_for the portable implementation of HQC-1). Several symbols that were not used across different source files were additionally declaredstatic.OQS common primitives replaced the included implementation of SHA3/Keccak and PRNG.
Name of algorithms have changed from HQC-128/192/256 to HQC-1/3/5.
Other:
src/commontest_kat.pytest so that on initial failure, the test binary is re-run to capture debugging outputKnown limitations
x86_64optimized implementations were not includedNo LLM/AI was used in this pull request