RE: [PATCH] Add x64 SHA-NI implementation to sha1 and sha256 in git ?

[<rsbecker@nexbridge.com>]

On Sunday, October 15, 2023 12:37 PM, Beat Bolli wrote:
>On 13.10.23 23:05, brian m. carlson wrote:
>> On 2023-10-12 at 20:04:47, Feiyang Xue wrote:
>>> Looking for comments on if this is a good idea to add SHA-NI option
>>> to git, And if so, what'd be a good implementation.
>>>
>>> The attached patch is more of a proof of concept,  using a sha-ni
>>> example found on internet and have it tapped into git's "hash-ll.h"
>>> when it sees make flags "SHA1_SHANI=1" and/or "SHA256_SHANI=1" for
>sha1/sha256 respectively.
>>>
>>> "git hash-object" is about 3-ish times faster for me
>>
>> You almost certainly don't want to use SHA-1 using native instructions
>> because it doesn't detect collisions, unlike the default
>> implementation (SHA-1-DC).  Since SHA-1 collisions are relatively
>> cheap (less than USD
>> 45,000) to make, not detecting collisions would be a security issue
>> worthy of a CVE.
>>
>> It's fine for SHA-256, but see below.
>>
>>> There are few topics that i'm uncertain about.
>>>
>>> 1. Is it good idea to have machine-specific asm codes in git. I read
>>> there was commit fd1ec82547 which removed assembly implementation for
>>> PPC_SHA1. Does that imply maintainers doesn't want machine-specific assembly
>in source?
>>
>> I'd prefer we didn't.
>>
>> Nettle has SHA-NI extensions on systems that support it, and so does
>> OpenSSL.  OpenSSL can't be used by distros for licensing reasons, but
>> Nettle can, and both of those libraries automatically detect the best
>> code to use based on the chip.  One of those libraries is almost
>> always going to be linked into Git at some point because of libcurl anyway.
>>
>> If we ship the code, then someone has to maintain it, and I don't know
>> if we have any assembly experts.  We might also have a bug that
>> produced incorrect results, which would be pretty disastrous for the
>> affected users.  It's much better for maintainability if we push that
>> off onto the cryptographic library maintainers who are much more
>> competent in that regard than I am (I will not speak for others) and
>> let distro maintainers simply link the appropriate library, which, as
>> I said above, they're already effectively doing.
>
>In addition to all the valid points brian makes, the patch uses a non-standard
>assembler (yasm, see http://yasm.tortall.net/) that's not part of the default GNU
>toolchain.

Aside from that, not everyone uses the GNU toolchain. I would suggest that if someone needs ASM support for SHA-1, they could reimplement SHA-1-DC using rotate ASM primitives instead of directly using SHA1 assembly, but I question whether that is significantly faster than having the compiler optimizer generate the equivalent rotates. Still, maintaining any ASM code requires out of box skillsets that are unlikely sustainable.
--Randall