Skip to content

Add Classic McEliece sanitization patch #2196

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aidenfoxivey
Copy link
Contributor

Signed-off-by: Aiden Fox Ivey [email protected]

@aidenfoxivey
Copy link
Contributor Author

@praveksharma

What's confusing me mainly is this bit:

~/s/pqclean % git status
HEAD detached at 1eacfdaf
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        modified:   crypto_kem/mceliece348864/avx2/controlbits.c
        modified:   crypto_kem/mceliece348864/clean/controlbits.c
        modified:   crypto_kem/mceliece348864f/avx2/controlbits.c
        modified:   crypto_kem/mceliece348864f/clean/controlbits.c
        modified:   crypto_kem/mceliece460896/avx2/controlbits.c
        modified:   crypto_kem/mceliece460896/clean/controlbits.c
        modified:   crypto_kem/mceliece460896f/avx2/controlbits.c
        modified:   crypto_kem/mceliece460896f/clean/controlbits.c
        modified:   crypto_kem/mceliece6688128/avx2/controlbits.c
        modified:   crypto_kem/mceliece6688128/clean/controlbits.c
        modified:   crypto_kem/mceliece6688128f/avx2/controlbits.c
        modified:   crypto_kem/mceliece6688128f/clean/controlbits.c
        modified:   crypto_kem/mceliece6960119/avx2/controlbits.c
        modified:   crypto_kem/mceliece6960119/clean/controlbits.c
        modified:   crypto_kem/mceliece6960119f/avx2/controlbits.c
        modified:   crypto_kem/mceliece6960119f/clean/controlbits.c
        modified:   crypto_kem/mceliece8192128/avx2/controlbits.c
        modified:   crypto_kem/mceliece8192128/clean/controlbits.c
        modified:   crypto_kem/mceliece8192128f/avx2/controlbits.c
        modified:   crypto_kem/mceliece8192128f/clean/controlbits.c

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   crypto_kem/hqc-128/clean/reed_solomon.c
        modified:   crypto_kem/hqc-192/clean/reed_solomon.c
        modified:   crypto_kem/hqc-256/clean/reed_solomon.c

(ignore how I accidentally did that to reed_solomon as well - it's not in the patch)

yet when I run copy_from_upstream.py, I get:

Schemes that differ from upstream:
Name: pqclean_mceliece348864_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece348864_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece348864f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece348864f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece460896_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece460896_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece460896f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece460896f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece6688128_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece6688128_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece6688128f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece6688128f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece6960119_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece6960119_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece6960119f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece6960119f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece8192128_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece8192128_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece8192128f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
Name: pqclean_mceliece8192128f_avx2, expected upstream: https://github.com/PQClean/PQClean.git - commit: 1eacfdafc15ddc5d5759d0b85b4cef26627df181
-----
~/s/pqclean % git remote get-url origin
https://github.com/PQClean/PQClean

I'm 90% sure I'm on the right commit and the right repository.

@aidenfoxivey
Copy link
Contributor Author

Here's an asciinema recording of what I did to get the original patch - maybe I'm doing something wrong?

https://asciinema.org/a/3zlbjQ7lp0woBeEYBHf4WNCrV

@dstebila
Copy link
Member

dstebila commented Jul 8, 2025

What you're doing makes sense. I'm wondering if they copy_from_upstream verify operation isn't properly applying the patches before doing the diff. But that also doesn't make sense, as that should cause problems in lots of places.

Out of curiosity, does anything improve if you change the False, False to True, True in the following line:

process_families(instructions, basedir, False, False)

@aidenfoxivey
Copy link
Contributor Author

What you're doing makes sense. I'm wondering if they copy_from_upstream verify operation isn't properly applying the patches before doing the diff. But that also doesn't make sense, as that should cause problems in lots of places.

Out of curiosity, does anything improve if you change the False, False to True, True in the following line:

process_families(instructions, basedir, False, False)

It seems that has this result:

Adding new KAT for Classic-McEliece-348864
Traceback (most recent call last):
  File "/Users/aidenfoxivey/src/liboqs/scripts/copy_from_upstream/copy_from_upstream.py", line 638, in process_families
    if kats['kem'][scheme['pretty_name_full']]['single'] != scheme['metadata']['nistkat-sha256']:
       ~~~~^^^^^^^
KeyError: 'kem'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/aidenfoxivey/src/liboqs/scripts/copy_from_upstream/copy_from_upstream.py", line 842, in <module>
    verify_from_upstream()
    ~~~~~~~~~~~~~~~~~~~~^^
  File "/Users/aidenfoxivey/src/liboqs/scripts/copy_from_upstream/copy_from_upstream.py", line 766, in verify_from_upstream
    process_families(instructions, basedir, True, True)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/aidenfoxivey/src/liboqs/scripts/copy_from_upstream/copy_from_upstream.py", line 643, in process_families
    if scheme['pretty_name_full'] not in kats['kem']:
                                         ~~~~^^^^^^^
KeyError: 'kem'

@aidenfoxivey
Copy link
Contributor Author

Sorry about the delay in additional work - just getting a bit loaded up with finals at the moment. I should be able to figure this out next week.

@aidenfoxivey aidenfoxivey force-pushed the main branch 2 times, most recently from 29704ac to c9eb3c5 Compare July 15, 2025 04:51
@aidenfoxivey aidenfoxivey requested a review from dstebila as a code owner July 15, 2025 05:24
@aidenfoxivey
Copy link
Contributor Author

Well this is quite fascinating - seems the flake is failing for once! Let me take a peek.

@aidenfoxivey
Copy link
Contributor Author

Okay, fixing

Signed-off-by: Aiden Fox Ivey <[email protected]>
@aidenfoxivey
Copy link
Contributor Author

tl;dr is just that I was daft lol

@coveralls
Copy link

Coverage Status

coverage: 82.758% (+0.005%) from 82.753%
when pulling 82c1a76 on aidenfoxivey:main
into 78e2389 on open-quantum-safe:main.

@aidenfoxivey
Copy link
Contributor Author

Alright, should be sorted!

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.

3 participants