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

Update Classic McEliece #1470

Merged
merged 11 commits into from Jul 17, 2023
Merged

Update Classic McEliece #1470

merged 11 commits into from Jul 17, 2023

Conversation

praveksharma
Copy link
Member

Fixes #1314 and takes into account discussion from #1459.

Changes SPDX line for src\common\pqclean_shims\crypto_declassify.h from MIT to CC0.

  • [Yes] 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.)
  • [No] 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 oqs-provider, OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on, @praveksharma ! Basically LGTM; it would be better if the old "*_vec" and "_avx" source files also already can be removed with this PR, though -- so that @dstebila doesn't have to do another "cleanup PR" again (as I failed to do this with Kyber90s :-/ :-)

@praveksharma
Copy link
Member Author

@baentsch, I've made a new commit which removes the old vec/avx directories.

baentsch
baentsch previously approved these changes May 18, 2023
@baentsch
Copy link
Member

Hmm -- @dstebila: do the remaining errors require a manual review of the suppression files or is it just a case of changed line numbers? If the former, who can do this? How long would that take? I wouldn't be particularly happy about holding 0.8.0 just for this. But having to do an 0.9.0 in a few days later because of the KAT changes also isn't appealing....

@dstebila
Copy link
Member

These errors actually aren't related to the constant time tests, they are compiler warnings and memory leak errors. So updating the suppression files won't help here. (We may also have to update the suppression files, but we'll only learn that when we run the constant time tests, which are not part of the regular CI process, only weekly or manual runs.)

I spent about an hour looking at the above failures this afternoon. I understand the warning in the scan-build job and I know that it's a false positive, though I don't know how to tell that to scan-build. As for the failures in test_kem_leak, I did not yet investigate those, so I don't know how much work it is to resolve those. I do not think I will have time to work on it on Friday.

@dstebila dstebila added this to the 0.9.0 milestone May 19, 2023
@baentsch baentsch dismissed their stale review June 12, 2023 07:47

Various changes since last review.

@praveksharma
Copy link
Member Author

The commit 8d16512 fixes the CI errors.

The scan_build issue is fixed by patching the Classic McEliece source code before copying it from PQClean. scan-build was complaining about an error that could've occurred if a variable went outside the range the algorithm allows it go. An assert statement telling scan-build that this is not possible gets it to stop complaining.

The memory leak issue is fixed by building Classic McEliece with lower optimisation, -01 instead of -03.

tests/test_leaks.py Outdated Show resolved Hide resolved
@baentsch
Copy link
Member

Thanks for these updates getting everything "green", @praveksharma !

The memory leak issue is fixed by building Classic McEliece with lower optimisation, -01 instead of -03.

This IMO begs one change to https://github.com/open-quantum-safe/liboqs/blob/main/docs/algorithms/kem/classic_mceliece.md#advisories: Something along the lines "clang reports memory leaks for all algorithms in this family when compiled with an optimization level greater than '-O1', so care is advised using this algorithm at higher optimization levels".

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more change request on this PR: The liboqs version number must be moved to "0.9.0-dev" in the toplevel CMakeLists.txt. Arguably also the "SOVERSION" (in src/CMakeLists.txt) -- but I'm not sure about that: Did we bump that when going to "0.8.0", @dstebila ?

@praveksharma
Copy link
Member Author

praveksharma commented Jul 11, 2023

d5d8ccd fixes the version number and the issues regarding the comments. That leaves us with deciding whether or not to bump the SOVERSION.

@dstebila
Copy link
Member

Let's increment the SOVERSION.

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through all these issues, @praveksharma ! Now LGTM.

@praveksharma praveksharma merged commit 0b64ca3 into open-quantum-safe:main Jul 17, 2023
12 checks passed
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.

Upgrade Classic McEliece to NIST round 4 specification
3 participants