Skip to content
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

crypto: chacha20: always use our fallback timingsafe_bcmp rather than libc's #29815

Merged
merged 1 commit into from Apr 6, 2024

Conversation

theuni
Copy link
Member

@theuni theuni commented Apr 5, 2024

Looking at libc sources, apple and openbsd implementations match our naive fallback. Only FreeBSD (and only x86_64) seems to implement an optimized version.

It's not worth the hassle of using a platform-specific function for such little gain.

Additionally, as mentioned below, this is the only case outside of sha2 that requires an autoconf check, and I have upcoming PRs to remove the sha2 ones.

Apple's impl is unoptimized.

As-is OpenBSD's impl.

Relevant IRC conversation with sipa:

<cfields> sipa: chacha20poly1305.cpp uses libc's timingsafe_bcmp when possible. But looking around at apple/freebsd/openbsd, I don't see any impl that doesn't use the naive implementation that matches our fallback...
<cfields> is there any reason to belive there's an optimized impl somewhere that we're actually hitting?
<cfields> asking because after cleaning up sha2, timingsafe_bcmp is the last autoconf check that remains in all of crypto. It'd make life easy if we could just always use our internal one.
<cfields> *all of crypto/
<sipa> cfields: let's get rid of the dependency then
<sipa> it's a trivial function
<sipa> and if we need it for some platforms, no real reason not to use it on all

After the above discusstion, I did end up finding the x86_64-optimized FreeBSD impl, but I don't think that's all that significant.

… libc's

Looking at apple/freebsd/openbsd sources, their implementations match our naive
fallback. It's not worth the hassle of using a platform-specific function for
no gain.
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 5, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK sipa, fanquake, TheCharlatan, theStack
Concept ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@theuni
Copy link
Member Author

theuni commented Apr 5, 2024

Ping @sipa for a quick concept ACK.

@sipa
Copy link
Member

sipa commented Apr 5, 2024

utACK 2d18194

@fanquake
Copy link
Member

fanquake commented Apr 5, 2024

Concept ACK

@hebasto
Copy link
Member

hebasto commented Apr 5, 2024

Concept ACK.

@theStack
Copy link
Contributor

theStack commented Apr 6, 2024

Concept ACK

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 2d18194

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK 2d18194

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK 2d18194

As a historical side-note, it seems like this function was first introduced for OpenSSH (managed in the OpenBSD tree) in 2010, with the name timing_safe_cmp back then: openbsd/src@8488487#diff-1baa12ad01bad68b45e89594ef3309ad070f9848f764f976e08c435ade846ae5R833

@fanquake fanquake merged commit 0f0e36d into bitcoin:master Apr 6, 2024
16 checks passed
Pttn added a commit to RiecoinTeam/Riecoin that referenced this pull request Apr 13, 2024
hebasto added a commit to hebasto/bitcoin that referenced this pull request Apr 18, 2024
hebasto added a commit to hebasto/bitcoin that referenced this pull request Apr 18, 2024
074fa1c fixup! cmake: Build `bitcoin_crypto` library (Hennadii Stepanov)

Pull request description:

  Backport changes from bitcoin#29815.

ACKs for top commit:
  theuni:
    utACK 074fa1c

Tree-SHA512: 975725ae45eafc666685c0d6ee67dc7600c8b75780d80f9c4e917bf19bc01507c3f9db51c1cd96edbe369c9a5429f66b04e1a9c5700a3e952bcdab332abcace8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants