Patch Kyber to fix ASAN error on ARM64

open-quantum-safe / liboqs

C library for prototyping and experimenting with quantum-resistant cryptography

https://openquantumsafe.org/

Other

1.91k stars 465 forks source link

Patch Kyber to fix ASAN error on ARM64 #1922

Closed praveksharma closed 2 months ago

praveksharma commented 2 months ago

Patch Kyber to fix address sanitiser issue on ARM64 as described in #1914. Patch copied from PQClean, in line with fix submitted in #1914.

[No] 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 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.)

praveksharma commented 2 months ago

Thank you for your work on #1914 @mingtaoy! I've copied the changes from PQClean as you pointed and added them as a patch. I've also added the authors from #1914 as co-authors. Would you please have a look at the changes?

baentsch commented 2 months ago

@praveksharma Thanks for pulling this over from PQClean. To make reviewing easier would you mind pointing to the actual PQClean commit you pulled over? This PR doesn't seem to be the identical logic as in #1914.

praveksharma commented 2 months ago

Sorry for the inadequate documentation @baentsch.

The changes were first introduced this commit. The logic is slightly different where instead of defining a function to round up integers this is done manually -- KYBER_SYMBYTES is 32 for Kyber 512, 768, and 1024 so KYBER_SYMBYTES + 2 + 6 is divisible by 8 while KYBER_SYMBYTES + 2 is not.

baentsch commented 2 months ago

Sorry for the inadequate documentation @baentsch.

No need to apologize; I've been too lazy to try to understand the logic (differences). So this LGTM then. So when this lands #1914 gets closed without merge: OK also for you @mingtaoy?

mingtaoy commented 2 months ago

@baentsch - Yes! When I worked on #1914, I was unaware at the time that this was already fixed upstream.