search

jedisct1 / libhydrogen

A lightweight, secure, easy-to-use crypto library suitable for constrained environments.
https://libhydrogen.org
Other
594 stars 88 forks source link

GCC 11 warnings read/accessing out of regions #123

Closed blocksebastian closed 2 years ago

blocksebastian commented 2 years ago

I just downloaded the Arm GNU Toolchain Version 11.2-2022.02 and want to cross-compile libhydrogen (current git). It builds and runs, but the warnings about accessing data out of region make me feel uncomfortable. System is a 32 Bit ARM.

Full build log

devshell bloc:/tmp/libhydrogen/build$ cmake --log-level=VERBOSE -DCMAKE_C_COMPILER=arm-none-linux-gnueabihf-gcc -DCMAKE_C_FLAGS="-mcpu=cortex-a9 -mfpu=vfpv3" -DCMAKE_SYSTEM_NAME=Linux ..
-- The C compiler identification is GNU 11.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /opt/gcc-arm-none-linux-gnueabihf-x86_64-festo-11.2-2022.02/bin/arm-none-linux-gnueabihf-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/libhydrogen/build
devshell bloc:/tmp/libhydrogen/build$ make 
Scanning dependencies of target hydrogen
[ 50%] Building C object CMakeFiles/hydrogen.dir/hydrogen.c.o
In file included from /tmp/libhydrogen/hydrogen.c:15:
/tmp/libhydrogen/impl/x25519.h: In function ‘hydro_x25519_ladder_part1’:
/tmp/libhydrogen/impl/x25519.h:232:5: warning: ‘hydro_x25519_mul’ reading 32 bytes from a region of size 4 [-Wstringop-overread]
  232 |     hydro_x25519_mul(z2, x2, hydro_x25519_a24, // z2 = E*a24
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  233 |                      sizeof(hydro_x25519_a24) / sizeof(hydro_x25519_a24[0]));
      |                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/libhydrogen/impl/x25519.h:232:5: note: referencing argument 3 of type ‘const hydro_x25519_limb_t *’ {aka ‘const unsigned int *’}
/tmp/libhydrogen/impl/x25519.h:141:1: note: in a call to function ‘hydro_x25519_mul’
  141 | hydro_x25519_mul(hydro_x25519_fe out, const hydro_x25519_fe a, const hydro_x25519_fe b, int nb)
      | ^~~~~~~~~~~~~~~~
In file included from /tmp/libhydrogen/hydrogen.c:19:
In function ‘hydro_sign_verify_p2’,
    inlined from ‘hydro_sign_verify_challenge’ at /tmp/libhydrogen/impl/sign.h:117:12,
    inlined from ‘hydro_sign_final_verify’ at /tmp/libhydrogen/impl/sign.h:178:12:
/tmp/libhydrogen/impl/sign.h:103:5: warning: ‘hydro_x25519_core’ accessing 160 bytes in a region of size 32 [-Wstringop-overflow=]
  103 |     hydro_x25519_core(&xs[0], challenge, pk, 0);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/libhydrogen/impl/sign.h: In function ‘hydro_sign_final_verify’:
/tmp/libhydrogen/impl/sign.h:103:5: note: referencing argument 1 of type ‘hydro_x25519_limb_t (*)[8]’ {aka ‘unsigned int (*)[8]’}
In file included from /tmp/libhydrogen/hydrogen.c:15:
/tmp/libhydrogen/impl/x25519.h:251:1: note: in a call to function ‘hydro_x25519_core’
  251 | hydro_x25519_core(hydro_x25519_fe xs[5], const uint8_t scalar[hydro_x25519_BYTES],
      | ^~~~~~~~~~~~~~~~~
In file included from /tmp/libhydrogen/hydrogen.c:19:
In function ‘hydro_sign_verify_p2’,
    inlined from ‘hydro_sign_verify_challenge’ at /tmp/libhydrogen/impl/sign.h:117:12,
    inlined from ‘hydro_sign_final_verify’ at /tmp/libhydrogen/impl/sign.h:178:12:
/tmp/libhydrogen/impl/sign.h:104:5: warning: ‘hydro_x25519_core’ accessing 160 bytes in a region of size 32 [-Wstringop-overflow=]
  104 |     hydro_x25519_core(&xs[2], sig, hydro_x25519_BASE_POINT, 0);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/libhydrogen/impl/sign.h: In function ‘hydro_sign_final_verify’:
/tmp/libhydrogen/impl/sign.h:104:5: note: referencing argument 1 of type ‘hydro_x25519_limb_t (*)[8]’ {aka ‘unsigned int (*)[8]’}
In file included from /tmp/libhydrogen/hydrogen.c:15:
/tmp/libhydrogen/impl/x25519.h:251:1: note: in a call to function ‘hydro_x25519_core’
  251 | hydro_x25519_core(hydro_x25519_fe xs[5], const uint8_t scalar[hydro_x25519_BYTES],
      | ^~~~~~~~~~~~~~~~~
In file included from /tmp/libhydrogen/hydrogen.c:19:
In function ‘hydro_sign_verify_core’,
    inlined from ‘hydro_sign_verify_p2’ at /tmp/libhydrogen/impl/sign.h:106:12,
    inlined from ‘hydro_sign_verify_challenge’ at /tmp/libhydrogen/impl/sign.h:117:12,
    inlined from ‘hydro_sign_final_verify’ at /tmp/libhydrogen/impl/sign.h:178:12:
/tmp/libhydrogen/impl/sign.h:74:5: warning: ‘hydro_x25519_ladder_part1’ accessing 160 bytes in a region of size 32 [-Wstringop-overflow=]
   74 |     hydro_x25519_ladder_part1(xs);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/libhydrogen/impl/sign.h: In function ‘hydro_sign_final_verify’:
/tmp/libhydrogen/impl/sign.h:74:5: note: referencing argument 1 of type ‘hydro_x25519_limb_t (*)[8]’ {aka ‘unsigned int (*)[8]’}
In file included from /tmp/libhydrogen/hydrogen.c:15:
/tmp/libhydrogen/impl/x25519.h:217:1: note: in a call to function ‘hydro_x25519_ladder_part1’
  217 | hydro_x25519_ladder_part1(hydro_x25519_fe xs[5])
      | ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/libhydrogen/hydrogen.c:19:
In function ‘hydro_sign_verify_core’,
    inlined from ‘hydro_sign_verify_p2’ at /tmp/libhydrogen/impl/sign.h:106:12,
    inlined from ‘hydro_sign_verify_challenge’ at /tmp/libhydrogen/impl/sign.h:117:12,
    inlined from ‘hydro_sign_final_verify’ at /tmp/libhydrogen/impl/sign.h:178:12:
/tmp/libhydrogen/impl/sign.h:81:5: warning: ‘hydro_x25519_mul’ reading 32 bytes from a region of size 4 [-Wstringop-overread]
   81 |     hydro_x25519_mul(z2, z2, &sixteen, 1);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/libhydrogen/impl/sign.h: In function ‘hydro_sign_final_verify’:
/tmp/libhydrogen/impl/sign.h:81:5: note: referencing argument 3 of type ‘const hydro_x25519_limb_t *’ {aka ‘const unsigned int *’}
In file included from /tmp/libhydrogen/hydrogen.c:15:
/tmp/libhydrogen/impl/x25519.h:141:1: note: in a call to function ‘hydro_x25519_mul’
  141 | hydro_x25519_mul(hydro_x25519_fe out, const hydro_x25519_fe a, const hydro_x25519_fe b, int nb)
      | ^~~~~~~~~~~~~~~~
[100%] Linking C static library libhydrogen.a
[100%] Built target hydrogen

Patches

The first is quite easy to patch as the number of elements is given to the function and hydro_x25519_a24 is only defined as an array with length 1.

diff --git a/impl/x25519.h b/impl/x25519.h
index efeb9a4..09bd96f 100644
--- a/impl/x25519.h
+++ b/impl/x25519.h
@@ -138,7 +139,7 @@ hydro_x25519_swapout(uint8_t *out, hydro_x25519_limb_t *x)
 }

 static void
-hydro_x25519_mul(hydro_x25519_fe out, const hydro_x25519_fe a, const hydro_x25519_fe b, int nb)
+hydro_x25519_mul(hydro_x25519_fe out, const hydro_x25519_fe a, const hydro_x25519_limb_t *b, int nb)
 {
     hydro_x25519_limb_t accum[2 * hydro_x25519_NLIMBS] = { 0 };
     hydro_x25519_limb_t carry2;

The other warnings are a little harder since it want hydro_x25519_fe[5] but there are different size of arrays. I'm not aware how to cast a "hydro_x25519_fe *" as it is used in hydro_sign_verify_p2 to a "hydro_x25519_fe[5]".

diff --git a/impl/x25519.h b/impl/x25519.h
index efeb9a4..09bd96f 100644
--- a/impl/x25519.h
+++ b/impl/x25519.h
@@ -214,7 +215,7 @@ hydro_x25519_canon(hydro_x25519_fe x)
 }

 static void
-hydro_x25519_ladder_part1(hydro_x25519_fe xs[5])
+hydro_x25519_ladder_part1(hydro_x25519_fe *xs)
 {
     hydro_x25519_limb_t *x2 = xs[0], *z2 = xs[1], *x3 = xs[2], *z3 = xs[3], *t1 = xs[4];

@@ -248,7 +249,7 @@ hydro_x25519_ladder_part2(hydro_x25519_fe xs[5], const hydro_x25519_fe x1)
 }

 static void
-hydro_x25519_core(hydro_x25519_fe xs[5], const uint8_t scalar[hydro_x25519_BYTES],
+hydro_x25519_core(hydro_x25519_fe *xs, const uint8_t scalar[hydro_x25519_BYTES],
                   const uint8_t *x1, bool clamp)
 {
     hydro_x25519_limb_t  swap;
solardiz commented 1 year ago

I'm not convinced this is in fact fixed. Here are the warnings I'm getting when using current hydrogen, gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) on x86_64 (here keygen.c is my program, into which hydrogen.c is included):

In file included from hydrogen/hydrogen.c:18,
                 from keygen.c:3:
In function 'hydro_sign_verify_p2',
    inlined from 'hydro_sign_verify_challenge' at hydrogen/impl/sign.h:117:12,
    inlined from 'hydro_sign_final_verify' at hydrogen/impl/sign.h:178:12:
hydrogen/impl/sign.h:103:5: warning: 'hydro_x25519_core' accessing 160 bytes in a region of size 32 []8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-overflow=-Wstringop-overflow=]8;;]
  103 |     hydro_x25519_core(&xs[0], challenge, pk, 0);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hydrogen/impl/sign.h: In function 'hydro_sign_final_verify':
hydrogen/impl/sign.h:103:5: note: referencing argument 1 of type 'hydro_x25519_limb_t (*)[4]' {aka 'long unsigned int (*)[4]'}
In file included from hydrogen/hydrogen.c:15,
                 from keygen.c:3:
hydrogen/impl/x25519.h:253:1: note: in a call to function 'hydro_x25519_core'
  253 | hydro_x25519_core(hydro_x25519_fe xs[5], const uint8_t scalar[hydro_x25519_BYTES],
      | ^~~~~~~~~~~~~~~~~
In file included from hydrogen/hydrogen.c:18,
                 from keygen.c:3:
In function 'hydro_sign_verify_p2',
    inlined from 'hydro_sign_verify_challenge' at hydrogen/impl/sign.h:117:12,
    inlined from 'hydro_sign_final_verify' at hydrogen/impl/sign.h:178:12:
hydrogen/impl/sign.h:104:5: warning: 'hydro_x25519_core' accessing 160 bytes in a region of size 32 []8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-overflow=-Wstringop-overflow=]8;;]
  104 |     hydro_x25519_core(&xs[2], sig, hydro_x25519_BASE_POINT, 0);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hydrogen/impl/sign.h: In function 'hydro_sign_final_verify':
hydrogen/impl/sign.h:104:5: note: referencing argument 1 of type 'hydro_x25519_limb_t (*)[4]' {aka 'long unsigned int (*)[4]'}
In file included from hydrogen/hydrogen.c:15,
                 from keygen.c:3:
hydrogen/impl/x25519.h:253:1: note: in a call to function 'hydro_x25519_core'
  253 | hydro_x25519_core(hydro_x25519_fe xs[5], const uint8_t scalar[hydro_x25519_BYTES],
      | ^~~~~~~~~~~~~~~~~
In file included from hydrogen/hydrogen.c:18,
                 from keygen.c:3:
In function 'hydro_sign_verify_core',
    inlined from 'hydro_sign_verify_p2' at hydrogen/impl/sign.h:106:12,
    inlined from 'hydro_sign_verify_challenge' at hydrogen/impl/sign.h:117:12,
    inlined from 'hydro_sign_final_verify' at hydrogen/impl/sign.h:178:12:
hydrogen/impl/sign.h:74:5: warning: 'hydro_x25519_ladder_part1' accessing 160 bytes in a region of size 32 []8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-overflow=-Wstringop-overflow=]8;;]
   74 |     hydro_x25519_ladder_part1(xs);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hydrogen/impl/sign.h: In function 'hydro_sign_final_verify':
hydrogen/impl/sign.h:74:5: note: referencing argument 1 of type 'hydro_x25519_limb_t (*)[4]' {aka 'long unsigned int (*)[4]'}
In file included from hydrogen/hydrogen.c:15,
                 from keygen.c:3:
hydrogen/impl/x25519.h:219:1: note: in a call to function 'hydro_x25519_ladder_part1'
  219 | hydro_x25519_ladder_part1(hydro_x25519_fe xs[5])
      | ^~~~~~~~~~~~~~~~~~~~~~~~~

and with gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04) on aarch64 (looks exactly the same):

In file included from hydrogen/hydrogen.c:18,
                 from keygen.c:3:
In function ‘hydro_sign_verify_p2’,
    inlined from ‘hydro_sign_verify_challenge’ at hydrogen/impl/sign.h:117:12,
    inlined from ‘hydro_sign_final_verify’ at hydrogen/impl/sign.h:178:12:
hydrogen/impl/sign.h:103:5: warning: ‘hydro_x25519_core’ accessing 160 bytes in a region of size 32 []8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-overflow=-Wstringop-overflow=]8;;]
  103 |     hydro_x25519_core(&xs[0], challenge, pk, 0);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hydrogen/impl/sign.h: In function ‘hydro_sign_final_verify’:
hydrogen/impl/sign.h:103:5: note: referencing argument 1 of type ‘hydro_x25519_limb_t (*)[4]’ {aka ‘long unsigned int (*)[4]’}
In file included from hydrogen/hydrogen.c:15,
                 from keygen.c:3:
hydrogen/impl/x25519.h:253:1: note: in a call to function ‘hydro_x25519_core’
  253 | hydro_x25519_core(hydro_x25519_fe xs[5], const uint8_t scalar[hydro_x25519_BYTES],
      | ^~~~~~~~~~~~~~~~~
In file included from hydrogen/hydrogen.c:18,
                 from keygen.c:3:
In function ‘hydro_sign_verify_p2’,
    inlined from ‘hydro_sign_verify_challenge’ at hydrogen/impl/sign.h:117:12,
    inlined from ‘hydro_sign_final_verify’ at hydrogen/impl/sign.h:178:12:
hydrogen/impl/sign.h:104:5: warning: ‘hydro_x25519_core’ accessing 160 bytes in a region of size 32 []8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-overflow=-Wstringop-overflow=]8;;]
  104 |     hydro_x25519_core(&xs[2], sig, hydro_x25519_BASE_POINT, 0);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hydrogen/impl/sign.h: In function ‘hydro_sign_final_verify’:
hydrogen/impl/sign.h:104:5: note: referencing argument 1 of type ‘hydro_x25519_limb_t (*)[4]’ {aka ‘long unsigned int (*)[4]’}
In file included from hydrogen/hydrogen.c:15,
                 from keygen.c:3:
hydrogen/impl/x25519.h:253:1: note: in a call to function ‘hydro_x25519_core’
  253 | hydro_x25519_core(hydro_x25519_fe xs[5], const uint8_t scalar[hydro_x25519_BYTES],
      | ^~~~~~~~~~~~~~~~~
In file included from hydrogen/hydrogen.c:18,
                 from keygen.c:3:
In function ‘hydro_sign_verify_core’,
    inlined from ‘hydro_sign_verify_p2’ at hydrogen/impl/sign.h:106:12,
    inlined from ‘hydro_sign_verify_challenge’ at hydrogen/impl/sign.h:117:12,
    inlined from ‘hydro_sign_final_verify’ at hydrogen/impl/sign.h:178:12:
hydrogen/impl/sign.h:74:5: warning: ‘hydro_x25519_ladder_part1’ accessing 160 bytes in a region of size 32 []8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-overflow=-Wstringop-overflow=]8;;]
   74 |     hydro_x25519_ladder_part1(xs);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hydrogen/impl/sign.h: In function ‘hydro_sign_final_verify’:
hydrogen/impl/sign.h:74:5: note: referencing argument 1 of type ‘hydro_x25519_limb_t (*)[4]’ {aka ‘long unsigned int (*)[4]’}
In file included from hydrogen/hydrogen.c:15,
                 from keygen.c:3:
hydrogen/impl/x25519.h:219:1: note: in a call to function ‘hydro_x25519_ladder_part1’
  219 | hydro_x25519_ladder_part1(hydro_x25519_fe xs[5])
      | ^~~~~~~~~~~~~~~~~~~~~~~~~
solardiz commented 1 year ago

@jedisct1 You'll probably want to reopen this issue until it's actually confirmed fixed. Thanks.

jedisct1 commented 1 year ago

@solardiz That looks like a bug in the analyzer.

1eee2b23f1bb170d4af155ea431ee8d0b6d9dde8 works around it.

solardiz commented 1 year ago

@jedisct1 Thank you for the prompt fix! I confirm it avoids the warnings on my two systems above.