Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Falcon constant time tests failing #1617

Closed
baentsch opened this issue Nov 29, 2023 · 7 comments · Fixed by #1646
Closed

Falcon constant time tests failing #1617

baentsch opened this issue Nov 29, 2023 · 7 comments · Fixed by #1646
Assignees
Milestone

Comments

@baentsch
Copy link
Member

The Falcon CT CI tests are failing since quite a few weeks coincidentally with the last commit (adding uninstall support -- but I somehow cannot imagine how this should impact CT for Falcon). Other thoughts what could be at play welcome.

@SWilson4
Copy link
Member

SWilson4 commented Dec 1, 2023

I think it's more likely that #1585 somehow caused the failures, as it touched Falcon source files, and the timing fits. However, as far as I can tell the only changes were to whitespace, so I'm still a little perplexed by the sudden breakage.

At first glance, it looks like at least some of the failures are indeed non-constant-time behaviour. The first one, for example, points to

if (f[u] >= lim || f[u] <= -lim
which I believe we would consider a "pass", based on the in-line documentation.

I'll be updating the Falcon code shortly to address #1561 / PQClean/PQClean#523 / #1608, so I suggest we delay fixing this after that's done. No sense in possibly having to update the constant-time issues / passes all over again after it lands. How does that sound @baentsch?

Not directly related to the Falcon failures, but worth noting: It seems like the "generic" run is picking up errors in the AVX2 functions. Is this the desired behaviour? I would have expected it to test only the "clean" implementations.

@SWilson4
Copy link
Member

SWilson4 commented Dec 1, 2023

Not directly related to the Falcon failures, but worth noting: It seems like the "generic" run is picking up errors in the AVX2 functions. Is this the desired behaviour? I would have expected it to test only the "clean" implementations.

Ah, I see why this is happening. See #1618.

@SWilson4 SWilson4 added this to the 0.10.0 milestone Dec 5, 2023
@SWilson4 SWilson4 self-assigned this Dec 8, 2023
@baentsch
Copy link
Member Author

Bad news: CT testing now has more failures than before -- but then again, we're testing more configs now...

@SWilson4
Copy link
Member

Bad news: CT testing now has more failures than before -- but then again, we're testing more configs now...

Especially given that it's no longer just Falcon failing but also BIKE. :( I haven't looked closely at the failures yet to see whether they constitute true non-CT behaviour or not. Will open an issue to track.

@cothan
Copy link
Member

cothan commented Dec 19, 2023

I'm very familiar with Falcon code base due to my work with Falcon aarch64. Can you give me a brief instruction how to enable constant time test locally so I can (hopefully) help out?

@baentsch
Copy link
Member Author

I'm very familiar with Falcon code base due to my work with Falcon aarch64. Can you give me a brief instruction how to enable constant time test locally so I can (hopefully) help out?

Thank you very much for this offer, @cothan ! All CT logic is visible in this CI test: In essence, you need a Linux image with valgrind installed (we have prepared "openquantumsafe/ci-ubuntu-focal-x86_64:latest" for this), build liboqs with the required instrumentation for the platform of choice (e.g., cmake args -DOQS_DIST_BUILD=OFF -DOQS_OPT_TARGET=generic -DCMAKE_BUILD_TYPE=Debug -DOQS_ENABLE_TEST_CONSTANT_TIME=ON) and execute the test script. At its beginning a full explanation is contained in comments.

@cothan
Copy link
Member

cothan commented Jan 2, 2024

So I made a PR #1646,, at my local test, it passed in AVX2 with haswell config, and Reference with generic config.
I don't see the constant time check is enabled in the PR CI/CD, can someone run enable the check at CI, or run it locally to confirm it?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants