Add uint128 support for HACL* curve25519 on Windows
Categories
(NSS :: Libraries, enhancement)
Tracking
(Not tracked)
People
(Reporter: kjacobs, Assigned: kjacobs)
References
Details
Attachments
(1 file)
Currently, GYP builds on Windows do not define have_int128_support since MSVC does not support the __int128 type [1]. This leads to a non-HACL* implementation of curve25519 being wired in [2].
We can, at minimum, use emulated int128 via KRML_VERIFIED_UINT128 (without any noticeable performance penalty) to switch back to HACL*. It's also worth looking into using intrinsics and/or Clang's implementation for a speedup.
[1] https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl.gyp#714,717
[2] https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/freebl_base.gypi#134
|
||
Comment 1•4 years ago
|
||
The way we deal with uint128 in the HACL* packaging is as follows.
- All 128-related functions are expected to be provided via
static inlinedefinitions in a header file - In types.h https://github.com/project-everest/hacl-star/blob/master/dist/kremlin/include/kremlin/internal/types.h#L69, we distinguish three cases:
- __int128 type exists
- MSVC on 64-bit machines
- everything else
- Each one of these cases triggers the inclusion of a different header that provides the expected static inline definitions.
For the first case, we use hand-written trivial implementations relying on built-in compiler support: https://github.com/project-everest/hacl-star/blob/master/dist/kremlin/kremlib/dist/minimal/fstar_uint128_gcc64.h
For the second case, we have a hand-written implementation (that is slightly outdated) relying on MSVC intrinsics: https://github.com/project-everest/hacl-star/blob/master/dist/kremlin/kremlib/dist/minimal/fstar_uint128_msvc.h -- this implementation needs some cleanup because it is interleaved with a fallback implementation we actually no longer use, and that needs to be removed
For the third case, we rely on the verified extracted implementation: https://github.com/project-everest/hacl-star/blob/master/dist/kremlin/kremlib/dist/minimal/FStar_UInt128_Verified.h
This recently underwent a round of cleanup after the ifdef maze was found to not really work on all of the platforms that Debian builds on. So you should be able to use that stuff more easily now. In particular, the second header seems to be what you need to make sure you have a fast uint128 implementation for MSVC. (The little cleanup to remove the fallback implementation would need to happen, along with a review of the hand-written code.)
Even better, if you take the latest version of types.h, the switch between all three versions should be automatic.
Hope this helps!
|
Assignee | |
Comment 2•4 years ago
|
||
@protz: that is very helpful, thanks for the explanation!
|
|
Assignee | |
Comment 3•4 years ago
|
||
This patch causes Windows 64-bit GYP builds to use HACL* curve25519 rather than the 32-bit (fiat-crypto) implementation.
We also no longer define KRML_VERIFIED_UINT128 on Windows and rely on types.h to set it when necessary. Specifically, clang/gcc builds use the compiler-provided __int128, MSVC uses intrinsics, and any others use the verified/emulated type.
This yields a ~5x speedup on Windows clang builds (as in Firefox).
|
|
||
Comment 4•4 years ago
|
||
Glad it works. If types.h has a bug and tries to enable uint128 support on win32, I'm happy to take a fix upstream -- for instance, could it be that https://github.com/project-everest/hacl-star/blob/master/dist/kremlin/include/kremlin/internal/types.h#L62 should be
(defined(_MSC_VER) && defined(_M_X64) && defined(__clang__)) || \
instead? I don't have a testing setup for clang-32 on windows so I'd be grateful for any testing you're able to do.
|
|
Assignee | |
Comment 5•4 years ago
|
||
(In reply to Jonathan Protzenko [:protz] from comment #4)
Glad it works. If types.h has a bug and tries to enable uint128 support on win32, I'm happy to take a fix upstream -- for instance, could it be that https://github.com/project-everest/hacl-star/blob/master/dist/kremlin/include/kremlin/internal/types.h#L62 should be
(defined(_MSC_VER) && defined(_M_X64) && defined(__clang__)) || \instead? I don't have a testing setup for clang-32 on windows so I'd be grateful for any testing you're able to do.
Yeah, that compound looks a bit odd... Inverting the _M_X64 check resolves the Fx build, but I need to do a little more testing then will open an upstream bug and/or PR (my Win32 VM is completely broken at the moment).
Pushed with our workaround for now: https://hg.mozilla.org/projects/nss/rev/566fa62d65225e98593e2caa58b592b2f1eeb4ba
Thanks for the additional context!
Description
•