Wrap pointers to s2n-bignum functions - delocator fix

[nebeid]

Issues:

Addresses CryptoAlg-2899

Description of changes:

The original issue was caused by assigning s2n-bignum functions directly to the members of a methods struct. This assignment uses adrp instructions at the assembly level. The delocator (in FIPS static build) replaces adrp with adr which has only +/- 1MB range of addressing. In some versions of gcc, the compiled code ends up exceeding that range between the instruction and its target address when it's in another assembly file such as s2n-bignum functions.

Wrapping those pointers to s2n-bignum functions does the following

• those wrapper functions are closer to where they're assigned to the members methods structs because they're in the same compilation unit.
• the body of these wrapper functions is simply a b branch instruction to the target label. The branch instruction has an address range of +/- 32MB. (The caller of the method member function will be responsible for the link register, i.e. where the control flow will continue when the function returns.)

Call-outs:

This suggests that we need to be careful not to assign external assembly functions directly to pointers (so that adrp is not used with them to store their relative address in a register) and we need to ensure they are called from another function that is nearby by wrapping them.

Testing:

On Graviton3, FIPS static build with gcc-11 which used to fail #2148 with

Building ASM object crypto/fipsmodule/CMakeFiles/bcm_hashunset.dir/bcm-delocated.S.o
FAILED: crypto/fipsmodule/CMakeFiles/bcm_hashunset.dir/bcm-delocated.S.o 
/usr/bin/gcc-11 -DBORINGSSL_FIPS -DBORINGSSL_HAVE_LIBUNWIND -DBORINGSSL_IMPLEMENTATION -DFIPS_ENTROPY_SOURCE_PASSIVE -Isymbol_prefix_include -I../include -Wno-newline-eof -Wa,--noexecstack -O3 -DNDEBUG -fPIC -MD -MT crypto/fipsmodule/CMakeFiles/bcm_hashunset.dir/bcm-delocated.S.o -MF crypto/fipsmodule/CMakeFiles/bcm_hashunset.dir/bcm-delocated.S.o.d -o crypto/fipsmodule/CMakeFiles/bcm_hashunset.dir/bcm-delocated.S.o -c crypto/fipsmodule/bcm-delocated.S
bcm.c: Assembler messages:
bcm.c:7811: Error: pc-relative address offset out of range
bcm.c:7813: Error: pc-relative address offset out of range
bcm.c:7815: Error: pc-relative address offset out of range

is passing with this change.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.97%. Comparing base (cc9c9f0) to head (d1452b1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2165      +/-   ##
==========================================
- Coverage   78.97%   78.97%   -0.01%     
==========================================
  Files         611      611              
  Lines      105552   105569      +17     
  Branches    14950    14949       -1     
==========================================
+ Hits        83361    83373      +12     
- Misses      21538    21542       +4     
- Partials      653      654       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jakemas]

Confirmed CI is now passing with this change applied on #2148

[jakemas]

Created #2166 to test this fix.

[jakemas]

Confirmed that FIPS static builds are all passing on #2166

[jakemas]

Merging this will unblock: #2148 which are required for FIPS compliance.

[andrewhop]

What is the difference between defining this wrapper function and how s2n-bignum defines bignum_add_p384? Is my mental model of C functions are equivalent to functions defined in the assembly files wrong?

[nebeid]

It's a valid question. This change stemmed from the observation that those 3 functions that were wrapped, when they were being assigned to the p521_methods struct in crypto/fipsmodule/ec/p521.c#L292-L296, they were creating the following (commented) assembly instructions that were replaced by delocate.go by the ones right underneath in <build-folder>/crypto/fipsmodule/bcm-delocated.S.

.Lp521_methods_init_local_target:
p521_methods_init:
...
// WAS adrp x10, :got:bignum_add_p521
adr x10, .Lbignum_add_p521_local_target
// WAS adrp x9, :got:bignum_sub_p521
adr x9, .Lbignum_sub_p521_local_target
// WAS adrp x6, :got:bignum_neg_p521
adr x6, .Lbignum_neg_p521_local_target

In the case of gcc compilations (not all versions, it depends which ones get lucky), the target address of the adr instructions was further than the admissible 1MB.
The observation that other s2n-bignum functions assigned to the same method such as bignum_sqr_p521_alt was accessed via bignum_sqr_p521_selector which is defined in the header file

aws-lc/third_party/s2n-bignum/include/s2n-bignum_aws-lc.h

Line 165 in 51ae4b1

static inline void bignum_mul_p521_selector(uint64_t z[S2N_BIGNUM_STATIC 9], const uint64_t x[S2N_BIGNUM_STATIC 9], const uint64_t y[S2N_BIGNUM_STATIC 9]) {

and wasn't causing the same problem, rather in bcm-delocated.S:

// WAS adrp	x7, bignum_sqr_p521_selector
	adr x7, .Lbignum_sqr_p521_selector_local_target

and not too far from it

.align 2
.p2align 4,,11
.type bignum_sqr_p521_selector, %function
.Lbignum_sqr_p521_selector_local_target:
bignum_sqr_p521_selector:
.LFB423:

.cfi_startproc
// WAS adrp x2, :got:OPENSSL_armcap_P
    sub sp, sp, 128
    stp x0, x30, [sp, #-16]!
    bl .LOPENSSL_armcap_P_addr
    mov x2, x0
    ldp x0, x30, [sp], #16
    add sp, sp, 128
// WAS ldr x2, [x2, #:got_lo12:OPENSSL_armcap_P]
    ldr w2, [x2]
    tst w2, 28672
    beq .L453
// WAS b bignum_sqr_p521_alt
    b .Lbignum_sqr_p521_alt_local_target

the b (branch) instructions on the last line doesn’t suffer from the same range limit as the adr instruction; b has a range of of +/- 32MB, while adr has a range of +/- 1MB.

That's why I thought that wrapping the s2n-bignum function which may be far with a "nearby" static function may result in the same pattern as with bignum_sqr_p521_selector

Before this PR, i.e. adding the wrappers, the initialiser of p521_methods was compiled to get the address of, e.g., bignum_add_p521 from an unknown location and assign it to a register, but delocator.go found that label within the module and considered it local, but was unluckily too far to be treated like other local labels.
Adding a wrapper function caused the address of a "closer" function to be put in the address and a branch instruction to that target, which the delocator treated still as local but didn't need to change the b instruction because it doesn't cause relocation like adrp does.

[justsmth]

This is probably expected, but I did try building this branch for x86-64 using gcc-14 and it's still failing as before:

error while processing "\t.section\t.data.rel.ro.local,\"aw\"\n" on line 226756: ".data section found in module"