rsa: use portable intrinsic for 64x64bit multiplication on Win ARM64

[tomato42]

_umul128() is x86_64 (x64) only, while __umulh() works both on x64 and ARM64

fixes #20234

Checklist

[tomato42]

@slproweb @jbekkema could you check if this fixes #20234 for you?

[slproweb]

"Fixes" is a relative term in the case of software. A fix for one problem can unexpectedly break something else.

Does the code once again compile and produce binaries for the VC-WIN64-ARM target? Yes.

Do the binaries load and run on Windows ARM64 hardware? Yes. Tested on Win10 ARM64. openssl.exe loads and runs. I also ran a few select items from the test suite: test\bntest.exe, test\rsa_test.exe, test\rsa_mp_test.exe, test\rsa_sp800_56b_test.exe. All output reported "ok." I don't know whether any of those test executables actually tested the inlined function with the new intrinsic, but the various executables successfully ran to completion.

Is it still functionally correct across all platforms, especially supported platforms? I don't know. Win + ARM64 is largely unsupported. Looking at what Microsoft says about __umulh() to multiply two 128-bit integers vs. the existing code, the change seems logically correct to me for handling multiply overflow. I don't know what the policy is of putting #include statements in the middle of code instead of somewhere nearer to the top of the file. I also don't know if the code should, on Intel x64 use the original _umul128() for any specific performance gains associated with that intrinsic (e.g. one multiply operation) with a fallback to __umulh() for Win + ARM64 (i.e. two multiply operations) and the fallback to the C implementation for everything else.

[tomato42]

@slproweb Thanks, I was asking about the compilation error specifically.

I'm the original author of the buggy code so I'm aware of the additional requirements of it. But good point about checking if the code is optimal; it's indeed not if the __umulh() is used on x64, msvc generates then two mul instructions (or imul and mul, more precisly) instead of one; _umul128 is necessary for optimal machine code on x64. Will fix it.

[beldmit]

LGTM

[tomato42]

The buildbot/master:windows-win10-x86_64 failure looks like a problem with the machine, not the PR

[paulidale]

My preference is for this version of the fix to go in.

[slproweb]

Nitpick: The most recent change that was introduced here makes the assumption that Microsoft will never experiment with other 64-bit architectures other than Intel x64 or ARM64.

https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#machine-types

Only lists a fraction of the hardware architectures Microsoft and Microsoft partners have experimented with over the years. Granted, only a select few ever made it as far as full VS toolchain support with real hardware running the Windows OS.

Everything non-Microsoft has the fallback position of the native C implementation. With the native C implementation, at least it'll compile but not run well. Throwing a warning about non-optimal code on all platforms that end up using the native C implementation seems better to me than just failing to compile altogether. But this has already been signed off on.

/EndNitpick

[tomato42]

That was intentional. Once you have such a system you will be at the best position to decide which option you want to pursue: use one of the existing intrinsics, use a new intrinsic, or fallback to pure C option.

The other platforms don't have the fallback not for new unexpected platforms, but rather for non-gcc, non-clang compilers that don't support 128 bit variables natively. So it will be used when it's both an uncommon platform and an uncommon compiler.

[slproweb]

Obviously a difference of opinion here but, IMO, code should aim to always compile successfully with the sole exception of security considerations, which I don't think applies here.

OpenSSL also has a no-asm option in Configure. Under a no-asm build, the code here still uses intrinsics. That certainly makes it more difficult to validate that the native C implementation is functionally correct on all platforms (especially MSVC), which could be problematic for things like FIPS validation.

Side note: There doesn't appear to be a CI test for no-asm.

The point is largely moot since this has already been accepted but not merged yet.

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[toonetown]

Has there been any further discussion on whether this or #20235 is the preferable approach? It seems to me from reading through them that this one probably is - but both PRs have been open and approved for a few days now.

[t8m]

Merged to all branches. Thank you for your contribution.

[t8m]

Obviously a difference of opinion here but, IMO, code should aim to always compile successfully with the sole exception of security considerations, which I don't think applies here.

OpenSSL also has a no-asm option in Configure. Under a no-asm build, the code here still uses intrinsics. That certainly makes it more difficult to validate that the native C implementation is functionally correct on all platforms (especially MSVC), which could be problematic for things like FIPS validation.

Side note: There doesn't appear to be a CI test for no-asm.

The point is largely moot since this has already been accepted but not merged yet.

@slproweb Any improvements are welcome. Please submit a PR if you have any changes you'd like to be merged.