Add MQOM

[rben-dev]

This PR adds the MQOM signature scheme. MQOM is part of on-ramp Round 2 NIST candidates, In order to avoid too many variants that would overload the library, we only provide implementations for GF(16) MQOM fast and short (with 3 or 5 rounds), as this is the best choice for a speed and signature size balance.

Three implementation profiles are integrated to liboqs:

• AVX2: this makes use of AVX2 and AES-NI extensions, suited for modern amd64 platforms.
• default: this is a portable implementation suitable for most desktop-like platforms (e.g. arm64 Mac M1, etc.)
• memopt: this is a portable implementation with memory optimizations that are more suitable for platforms with stringent memory constraints (e.g. embedded platforms or targets with small stacks, etc.)

Here are some notable elements for this MQOM integration:

• MQOM uses Keccak, and we use the liboqs API for this.
• MQOM in security level 1 uses AES-128, and we use liboqs API for this. However, for security levels 3 and 5 Rijndael-256-256 is used, and hence we had to provide local implementations for this (see in the rijndael/ folder upstream).
• MQOM uses a large stack in the AVX2 and default implementations:
  • We had to deactivate the threaded signature tests in tests/test_sig.c. This is particularly useful for Mac OS platforms where the threads stacks is quite small.
  • We also had to adapt the tests/test_leaks.py file by providing --max-stackframe=20480000 to valgrind as discussed in issue Maximum stack usage for valgrind "leak test" #2360 : without this the CI tests fail with false positives. The strategy for this fix can be of course discussed. The fix is part of the PR.

Many elements of this PR have been produces with the copy_from_upstream.py automation (thanks for this): I only had to format with astyle the produced files in src/sig/mqom/ root.

Thanks in advance for considering the inclusion of MQOM in the library, as we believe this can be useful for exploring on-ramp candidates. Please do not hesitate if anything is wrong or missing!

• Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
• Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged. Also, make sure to update the list of algorithms in the continuous benchmarking files: .github/workflows/kem-bench.yml and sig-bench.yml)

[baentsch]

Hi @rben-dev -- thanks very much for bringing up this PR. For starters, could you please fix up the DCO error message as per CI help messages? I also triggered the other CI jobs to see how things look like before taking a deeper look into your contribution.

[rben-dev]

Hi @baentsch,

Thank you very much for your prompt reply and help. The DCO issue should be resolved.

However, from the CI failures:

• For the CBOM check, what is expected?
• For the Verify copy_from_upstream state after copy, the issue seems to be in the usage of supported-platforms: all in my META files for the portable implementations (default and memopt). I switched to this because the expected supported_platforms key triggers an error (at least on my Linux distro) when applied locally:

  File "liboqs/scripts/copy_from_upstream/copy_from_upstream.py", line 911, in <module>
    copy_from_upstream(slh_dsa_instruction)
    ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "liboqs/scripts/copy_from_upstream/copy_from_upstream.py", line 759, in copy_from_upstream
    process_families(instructions, os.environ['LIBOQS_DIR'], True, True)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "liboqs/scripts/copy_from_upstream/copy_from_upstream.py", line 661, in process_families
    if req['architecture'] == 'arm_8':
       ~~~^^^^^^^^^^^^^^^^
TypeError: string indices must be integers, not 'str'

The local CI on my fork was happy with the original commits, although not performing these basic checks for some unkown reason ... I am really sorry if I have missed something obvious here.

Thanks in advance,

[rben-dev]

For completeness regarding the supported platforms flag, I actually inspired my META yml from the SNOVA META here:

• The generic opt implementation uses supported-platforms with all : https://github.com/vacuas/SNOVA/blob/65a4f79effd084982401286b1f92184f64a36929/liboqs/META/SNOVA_24_5_16_4_AES_META.yml#L28
• The specific avx2 implementation uses supported_platforms with platforms details : https://github.com/vacuas/SNOVA/blob/65a4f79effd084982401286b1f92184f64a36929/liboqs/META/SNOVA_24_5_16_4_AES_META.yml#L38

I guess the issue might lie within the fact that the MQOM META provides two generic implementations that can run on all platforms, hence an ambiguity about the default implementation? Then, how is it possible to include two such generic implementations leaving the choice of the one included in the compiled lib to the user at compilation time?

[bhess]

Thanks for the contribution @rben-dev!

• For the CBOM check, what is expected?
• For the Verify copy_from_upstream state after copy, the issue seems to be in the usage of supported-platforms: all in my META files for the portable implementations (default and memopt). I switched to this because the expected supported_platforms key triggers an error (at least on my Linux distro) when applied locally:

Both CI failures are related to the same, liboqs assumes that there is a single portable implementation.

I've opened draft PR #2367 which adds explicit support for memory-optimized implementations, and fixes the CBOM issue. The change is that an implementation can now be declared as memory-optimized by adding memory_optimized: true to its entry in the upstream META.yml. The cmake build automatically compiles and prioritizes it when the user builds with -DOQS_MEMOPT_BUILD=ON.

Feel free to rebase on top of #2367, with memory_optimized: true on the memopt entries in your META.yml files. Please let me know if you run in to any issues.

[rben-dev]

Hi @bhess,

Thank you very much for this PR, adding the new memopt feature, and the explanations.

I have updated the current PR with a rebase, my local tests seem to confirm that the MQOM memopt variant compiles well. Now hoping that the full CI passes.

Regards,

[rben-dev]

It seems that the basic-checks / Check upstream code is properly integrated fails. This might be due to the fact that I have manually reformatted the generated files src/sig/mqom/sig_mqom_mqom2_*.c after the copy_from_upstream.py execution, because the CI formatting check would fail if I do not reformat them (at least it failed on my local CI tests and during the CI tests on my fork).

Any idea on how to deal with this code formatting issue in the CI without breaking one of these two checks?

Thanks in advance,

[bhess]

Thanks for the update @rben-dev.

It seems that the basic-checks / Check upstream code is properly integrated fails. This might be due to the fact that I have manually reformatted the generated files src/sig/mqom/sig_mqom_mqom2_*.c after the copy_from_upstream.py execution, because the CI formatting check would fail if I do not reformat them (at least it failed on my local CI tests and during the CI tests on my fork).

It seems the templating code in #2367 formats the memory‑optimized code incorrectly. I’ve updated #2367 with a fix. Feel free to sync your PR with it.

[rben-dev]

Thanks @bhess for the formatting fix in the generated code!
I rebased with your updated PR to integrate it in the current PR.

[rben-dev]

Hi @bhess,

The Zephyr build seems to be the only one failing in the CI, and this is for a good reason as I forgot to add MQOM there. I made an attempt for this, but this raises a question:

• Because of the memory constraints (large stack), only the memopt variant (for MQOM) is allowed on Zephyr, which made me put set(OQS_MEMOPT_BUILD ON) in the zephyr/CMakeLists.txt: b83ff03#diff-9806c6d45d5930b1bc23048b99f197ffdd4346320a1161cc5a501a5292006b6aR150
• However, this might be a too broad option as this will be applied for all the algorithms, which might be unwanted for other algorithms ... We might want more fine grained activations for the memopt option per algorithm and/or variant, without the global OQS_MEMOPT_BUILD option set (?)

Please correct me if I have missed some magical options to activate to deal with this. Another temporary solution to advance the PR would be to completely deactivate MQOM for Zephyr.

Thanks in advance for your help with this,

[bhess]

• However, this might be a too broad option as this will be applied for all the algorithms, which might be unwanted for other algorithms ... We might want more fine grained activations for the memopt option per algorithm and/or variant, without the global OQS_MEMOPT_BUILD option set (?)

To me, this sounds like a reasonable default for an embedded target like Zephyr. Perhaps @Frauschi has an opinion on this, as they integrated the Zephyr build?

[Frauschi]

• However, this might be a too broad option as this will be applied for all the algorithms, which might be unwanted for other algorithms ... We might want more fine grained activations for the memopt option per algorithm and/or variant, without the global OQS_MEMOPT_BUILD option set (?)

To me, this sounds like a reasonable default for an embedded target like Zephyr. Perhaps @Frauschi has an opinion on this, as they integrated the Zephyr build?

Yeah, sounds reasonable to me, too. At the time I wrote the Zephyr port, OQS_MEMOPT_BUILD wasn‘t even available if I remember correctly, but that suits perfectly.

[rben-dev]

Thanks for the feedback @bhess and @Frauschi :

• I guess that globally activating the new OQS_MEMOPT_BUILD toggle seems reasonable for all Zephyr build then. @bhess : if you agree I can fix my patch to move this activation outside MQOM for a global scope in zephyr/CMakeLists.txt?
• The CI spotted a weird thing: Zephyr v3.7.0 passes, but Zephyr v3.4.0 does not (https://github.com/open-quantum-safe/liboqs/actions/runs/22321345446/job/64680133394?pr=2361). After spending some time debugging with qemu and gdb, I have isolated the root cause (or at least one of them): this seems related to a bad compilation of the calloc function in the Zephyr v3.4.0 firmware where it ends up in a recursive call instead of calling malloc (which is documented in a fixed issue of the Zephyr project: gcc optimizes calloc to an infinite recursive call when CONFIG_SPEED_OPTIMIZATION=y zephyrproject-rtos/zephyr#64941, maybe explaining why this does not happen on Zephyr v3.7.0). MQOM triggers this in the CI because it seems to be the only algorithm that uses calloc among the ones tested there ...

I do not know how to deal with this last point in the most efficient way:

• It seems rather excessive to modify the MQOM code to not use the standard calloc (and use malloc + memset instead). But if this is the only left solution I might consider it.
• I could also completely remove MQOM from the Zephyr build, but this also seems a bit extreme given the reasons for this (i.e. a problem with the Zephyr toolchain in v3.4.0).
• A last possibility would be to adapt the Zephyr v3.4.0 build to integrate the fixes (libc: Add GCC fno-builtin-malloc flag to common stdlib compilation zephyrproject-rtos/zephyr#65082), but this would necessitate some work on the CI. Maybe @Frauschi has some idea wrt Zephyr fixes?

Thanks in advance for your advice and guidance with this,

[Frauschi]

I created a PR to fix the Zephyr CI problems (#2369). As version 3.4.0 has been EOL for a long time, we can safely drop it. Instead, the latest version (4.3.0) should be added to the CI tests. This should hopefully resolve your issues, too.

[bhess]

• I guess that globally activating the new OQS_MEMOPT_BUILD toggle seems reasonable for all Zephyr build then. @bhess : if you agree I can fix my patch to move this activation outside MQOM for a global scope in zephyr/CMakeLists.txt?

I agree.

I created a PR to fix the Zephyr CI problems (#2369). As version 3.4.0 has been EOL for a long time, we can safely drop it. Instead, the latest version (4.3.0) should be added to the CI tests. This should hopefully resolve your issues, too.

Great, thanks for this PR!

[rben-dev]

Hi,

Thanks @Frauschi for the new PR!
I have rebased mine to use it. @bhess : I have activated OQS_MEMOPT_BUILD globally for Zephyr.

With the Zephyr PR, my local CI tests now pass with MQOM.
Waiting and hoping that everything will be OK with the upstream CI.

Regards,

[rben-dev]

Same as in PR #2369, the Zephyr CI now passes with MQOM but for unknown reason the coverage tests are now failing ...

[rben-dev]

@bhess: one point that can be of interest for the future PR (if I did not miss something). I had to comment this line in the copy_from_upstream script when experimenting with common_meta_path to overcome a Python error:

liboqs/scripts/copy_from_upstream/copy_from_upstream.py

Line 473 in ab0e07c

print("Obtain files for %s" % (scheme))

When commented, everything else was fine (except for the compilation step of course because of the discussed CFLAGS issue ...).

[bhess]

Adding this include_only: true sounds great, please let me know when the draft PR lands so that I can experiment with it.

I’ve added draft PR #2382 implementing this functionality. For usage examples, see https://github.com/bhess/MAYO-C/blob/bhe-oqscommon/META/COMMONS.yml and https://github.com/bhess/MAYO-C/blob/bhe-oqscommon/META/MAYO-1_META.yml.

[rben-dev]

@bhess : thanks for the PR, this seems to work :-) I have pushed an attempt to use this in my last commit rebased on your PR #2382, hoping that the CI still passes.

By the way, beware that there is still an issue in:

liboqs/scripts/copy_from_upstream/copy_from_upstream.py

Line 473 in ab0e07c

print("Obtain files for %s" % (scheme))

in verbose mode (which is why CI might not hit this error): scheme is not defined here.

EDIT: in order to avoid all the history with all the unwanted files, I have force pushed a "clean" import above the rebased PR.

[bhess]

@bhess : thanks for the PR, this seems to work :-) I have pushed an attempt to use this in my last commit rebased on your PR #2382, hoping that the CI still passes.

By the way, beware that there is still an issue in:

liboqs/scripts/copy_from_upstream/copy_from_upstream.py

Line 473 in ab0e07c

print("Obtain files for %s" % (scheme))

in verbose mode (which is why CI might not hit this error): scheme is not defined here.

EDIT: in order to avoid all the history with all the unwanted files, I have force pushed a "clean" import above the rebased PR.

Thanks for trying this, reducing the change from over 2000 files down to just 144 additions is a great improvement. I’ve also removed the offending line in the script.

[rben-dev]

Hi,

Thanks for the feedback, the reduction in the number of files is great.
@bhess: the platform-tests in the CI seem OK. How to activate the extended tests? (I have the [extended tests] magic in the commit message)
@xuganyu96 : are these changes OK to perform the review?

Regards,

[xuganyu96]

Thank you for the file de-duplication efforts. This pull request is so much cleaner and easier to review!

I have one remaining concern: it looks like there are some stale diff's from an older pull request that got mixed into your pull request (see individual comments for some examples, though they might not be complete). I tried rebasing your branch on top of main, but these diffs persisted. Could you please check the commit history? Alternatively, you can re-run copy_from_upstream.py from a clean copy of main, then do a force-push to this branch (or raise a new pull request).

[xuganyu96]

This looks like a stale commit from an older pull request.

[xuganyu96]

This also looks like a stale commit from the same older pull request

[rben-dev]

Sorry if things got mixed with all the other PRs and rebasing ...
If you agree, I will open a new PR with a clean state from the current main.