Skip to content

CBMC: Add proof and contract for mldsa_shake128_stream_init #200

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

Merged
merged 1 commit into from
May 3, 2025

Conversation

jakemas
Copy link
Contributor

@jakemas jakemas commented May 2, 2025

Resolves #141

@jakemas jakemas requested a review from a team as a code owner May 2, 2025 19:04
@jakemas jakemas force-pushed the mldsa_shake128_stream_init branch from 1f9db18 to 52364de Compare May 2, 2025 19:08
Copy link
Contributor

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

One unnecessary use of memory_no_alias in the preconditions.

@jakemas
Copy link
Contributor Author

jakemas commented May 2, 2025

One unnecessary use of memory_no_alias in the preconditions.

I see, you are correct. When I remove this precondition I get an issue with CBMC overflow detection:

VERIFICATION FAILED

[MLD_65_ref_mldsa_shake128_stream_init.overflow.1] line 15 arithmetic overflow on unsigned to unsigned type conversion in (uint8_t)nonce: FAILURE

@hanno-becker
Copy link
Contributor

hanno-becker commented May 2, 2025

@jakemas Yes, CBMC does not like implicit truncation. You need to explicitly mask: t[0] = nonce & 0xFF

Have a look at https://github.com/pq-code-package/mlkem-native/blob/main/mlkem/compress.c#L101 where the same thing happened during compression in mlkem-native.

@jakemas
Copy link
Contributor Author

jakemas commented May 2, 2025

@jakemas Yes, CBMC does not like implicit truncation. You need to explicitly mask: t[0] = nonce & 0xFF

Have a look at https://github.com/pq-code-package/mlkem-native/blob/main/mlkem/compress.c#L101 where the same thing happened during compression in mlkem-native.

Got it! Thanks!

@jakemas jakemas force-pushed the mldsa_shake128_stream_init branch from f5715ad to 61649d8 Compare May 2, 2025 19:40
@jakemas jakemas force-pushed the mldsa_shake128_stream_init branch from 3527674 to 27ebd64 Compare May 2, 2025 19:42
@jakemas jakemas force-pushed the mldsa_shake128_stream_init branch from 3e3e284 to 86ad4c1 Compare May 2, 2025 19:45
Copy link
Contributor

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

Function arguments T foo[N] must be declared as T* foo in the harness, not as T foo[N]. Otherwise, the function will only be considered for valid foo, even if it does not have a precondition for foo.

@jakemas Can you please check existing harnesses to ensure we are not doing this elsewhere? We should never allocate an array in a harness.

@jakemas jakemas force-pushed the mldsa_shake128_stream_init branch from 0646741 to 53aebb3 Compare May 2, 2025 19:48
Copy link
Contributor

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

Thanks @jakemas!

@jakemas jakemas changed the title CBMC:Add proof and contract for mldsa_shake128_stream_init CBMC: Add proof and contract for mldsa_shake128_stream_init May 2, 2025
@mkannwischer mkannwischer merged commit 13fa708 into main May 3, 2025
51 checks passed
@mkannwischer mkannwischer deleted the mldsa_shake128_stream_init branch May 3, 2025 03:16
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.

CBMC: Prove mldsa_shake128_stream_init
4 participants