From 651ecf5b589f7ba31ef1908183fd23468ea1331a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 27 Jan 2024 13:24:51 -0500 Subject: [PATCH] Rewrite bn_big_endian_to_words to avoid a GCC false positive I got a -Wstringop-overflow warning in GCC 12.2.0, targetting 32-bit Arm. It's a false positive, but rewriting the function this way seems a bit clearer. (Previously, I tried to write it in a way that truncated if the bounds were wrong. Just make it a BSSL_CHECK.) Change-Id: Iaa3955f08f320f2c376ca66703e4dd29481128fd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65867 Reviewed-by: Bob Beck Commit-Queue: David Benjamin Auto-Submit: David Benjamin (cherry picked from commit 15a76eb224ec4eff94d00565ee7d13b1f5a3a6cc) --- crypto/fipsmodule/bn/bytes.c | 37 +++++++++++++++++++-------------- crypto/fipsmodule/bn/internal.h | 2 +- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/crypto/fipsmodule/bn/bytes.c b/crypto/fipsmodule/bn/bytes.c index 9a06ba7837e..e222f2f6853 100644 --- a/crypto/fipsmodule/bn/bytes.c +++ b/crypto/fipsmodule/bn/bytes.c @@ -63,26 +63,31 @@ void bn_big_endian_to_words(BN_ULONG *out, size_t out_len, const uint8_t *in, size_t in_len) { - for (size_t i = 0; i < out_len; i++) { - if (in_len < sizeof(BN_ULONG)) { - // Load the last partial word. - BN_ULONG word = 0; - for (size_t j = 0; j < in_len; j++) { - word = (word << 8) | in[j]; - } - in_len = 0; - out[i] = word; - // Fill the remainder with zeros. - OPENSSL_memset(out + i + 1, 0, (out_len - i - 1) * sizeof(BN_ULONG)); - break; - } + // The caller should have sized |out| to fit |in| without truncating. This + // condition ensures we do not overflow |out|, so use a runtime check. + BSSL_CHECK(in_len <= out_len * sizeof(BN_ULONG)); + // Load whole words. + while (in_len >= sizeof(BN_ULONG)) { in_len -= sizeof(BN_ULONG); - out[i] = CRYPTO_load_word_be(in + in_len); + out[0] = CRYPTO_load_word_be(in + in_len); + out++; + out_len--; } - // The caller should have sized the output to avoid truncation. - assert(in_len == 0); + // Load the last partial word. + if (in_len != 0) { + BN_ULONG word = 0; + for (size_t i = 0; i < in_len; i++) { + word = (word << 8) | in[i]; + } + out[0] = word; + out++; + out_len--; + } + + // Fill the remainder with zeros. + OPENSSL_memset(out, 0, out_len * sizeof(BN_ULONG)); } BIGNUM *BN_bin2bn(const uint8_t *in, size_t len, BIGNUM *ret) { diff --git a/crypto/fipsmodule/bn/internal.h b/crypto/fipsmodule/bn/internal.h index a71c553e3b0..11c68253d5a 100644 --- a/crypto/fipsmodule/bn/internal.h +++ b/crypto/fipsmodule/bn/internal.h @@ -774,7 +774,7 @@ void bn_mod_inverse0_prime_mont_small(BN_ULONG *r, const BN_ULONG *a, // unsigned integer and writes the result to |out_len| words in |out|. The output // is in little-endian word order with |out[0]| being the least-significant word. // |out_len| must be large enough to represent any |in_len|-byte value. That is, -// |out_len| must be at least |BN_BYTES * in_len|. +// |in_len| must be at most |BN_BYTES * out_len|. void bn_big_endian_to_words(BN_ULONG *out, size_t out_len, const uint8_t *in, size_t in_len);