Implement IBT on amd64 / i386

[lgv5]

Description of changes: as the title reads, this sprinkles endbr64 / endbr32 as needed in the assembler files to be able to run with CET enabled. I tested this on an i7 1165G7 on OpenBSD, as CET is enabled by default there. I do get SIGILLs without the patch, and the testsuite passes with it. The PR affects quite a lot of files as it basically adds a _CET_ENDBR after each function definition. The following script was used for patching the files:

#!/bin/sh
find . -type f -name '*.S' -exec grep -lx 'S2N_BN_SYMBOL(.*):' {} + |
    while read -r f; do
        sed -i -f /dev/stdin "$f" <<'EOF'
/^S2N_BN_SYMBOL([^)]*):$/ {
        :Loop
        n
        /^S2N_BN_SYMBOL([^)]*):$/ b Loop
        i\
        _CET_ENDBR
}
EOF
done

The sed machinery is only needed to avoid adding multiple _CET_ENDBRs in definitions like

S2N_BN_SYMBOL(curve25519_x25519base):
S2N_BN_SYMBOL(curve25519_x25519base_byte):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[lgv5]

Alternatively, I can introduce

#define S2N_BN_SYM_FUNCTION(name) \
S2N_BN_SYMBOL(name): \
  _CET_ENDBR

in include/_internal_s2n_bignum.h and replace all the stray S2N_BN_SYMBOL(name): with it, but that also doesn't escape of the complexity of a 629-files diff. The upside of this, tho, is that future additions wouldn't have to worry about _CET_ENDBR as long as they use the new define.

[jargh]

Thank you for addressing this whole issue, which as far as I know had passed us by completely. We should probably be doing more testing on BSD versions!

Your solution seems to be a nice lightweight one in the sense that the additional ENDBR instructions are only added with __CET__ enabled, and otherwise things are unchanged. This has the pleasant effect that the proofs don't need to be changed :-)

Of course, it is unfortunate that the proofs in principle don't cover the CET-enabled variants. We could adopt a more heavyweight solution of just adding ENDBR instructions regardless, trusting them to have minimal performance impact and treating them as NOPs. According to the Intel doc quoted in the discussion here they can safely be modeled as a NOP and assumed to behave as such even on legacy processors:

https://stackoverflow.com/questions/56905811/what-does-the-endbr64-instruction-actually-do

My inclination is to go with your solution first and maybe think about the more far-reaching change in the future. Any other opinions?

[lgv5]

Thank you for addressing this whole issue, which as far as I know had passed us by completely. We should probably be doing more testing on BSD versions!

It was a pleasure, compared to other projects where I did IBT patching. The only bothersome part is the amount of files to patch. :D

AFAIK, only OpenBSD enforces CET, with Intel CPUs from gen 11th and onwards. Also, we only do IBT, not full CET (we don't do shadow stacks).

My inclination is to go with your solution first and maybe think about the more far-reaching change in the future. Any other opinions?

After I thought more about it, from a maintainer point of view, I think the approach of the comment (defining a macro that outputs a function preamble) is better, in the sense that there is no need for another 629-files patch. But well, I'm not a maintainer. I'm still up for biting the bullet again, if you prefer that approach.

[lgv5]

Hello @jargh ! Are there any updates in here? We really we can merge it so it can eventually backpropagate to aws-lc-rs/aws-lc-sys, as more things in Rust land are depending on it.

[jargh]

Sorry for dropping the ball on this. I don't think we'll have time to go for the more complex solution of actually formally modeling the endbr instructions very soon (i.e. for a few months). So in the light of that, I think we'll go, at least for the short term, with your solution pretty much "as is", i.e. modifying the syntax when the appropriate build flags are set. Would you mind checking if it needs rebasing w.r.r. any other updates and then I'll prioritize merging it in.

[lgv5]

Hey jargh! I rebased the PR onto main, and I added a reentrant version of the script, which should help to avoid missing _CET_ENDBR instructions. It can become specially handy on CI: I believe that adding

sh add-ibt-endbr.sh && git diff --exit-code .

after https://github.com/awslabs/s2n-bignum/blob/main/codebuild/tests.yml#L12 should do the trick. Feel free to drop it if it isn't desired.

[jargh]

Many thanks! I'm working on assimilating and checking it. I'll also experiment a bit to see what the performance hit seems to be, because if it's not much (and assuming there are no other issues) we really may want to prioritize making this the default and updating the proofs accordingly.

[jargh]

Since all our code is assuming 64-bit mode, I suspect we're safe ignoring ENDBR32, right? (For example, not including it in x86/allowed_asm.) Or would it still be relevant in some situation?

I'm actually somewhat warming to the idea of making this the default and updating the proofs, but let me do a bit more investigating and poll some other people about what they think.