Add call frame information (CFI) annotations

[jargh]

Issue #, if available: #197

Description of changes:

This includes call frame information annotations in all the code, roughly following the explanation here: https://www.imperialviolet.org/2017/01/18/cfi.html

The change is made for both ARM and x86, largely by wrapping certain relevant instructions in macros that augment the core instruction with appropriate CFI directives, e.g. for ARM

#define CFI_PUSH2(lo,hi) stp lo, hi, [sp, #-16]! ; .cfi_adjust_cfa_offset 16 ; .cfi_rel_offset lo, 0 ; .cfi_rel_offset hi, 8

and for x86

#define CFI_PUSH(reg) push reg ; .cfi_adjust_cfa_offset 8 ; .cfi_rel_offset reg, 0

As part of this, two other systematic changes are made:

• All local labels are prefixed with L to make their local-ness explicit
• The internal header is split into separate ones for arm, x86 and x86_att

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

[briansmith]

Have you tried using objtool to verify the annotations? See https://github.com/torvalds/linux/blob/3a90a72aca0a98125f0c7350ffb7cc63665f8047/tools/objtool/Documentation/objtool.txt for documentation on it. My understanding from reading the LKML is that it was expressly designed to support non-Kernel use cases too.

[jargh]

Thanks, objtool is very interesting, and I didn't know about it at all. I've built it and started to play around with it on this CFI-enabled s2n-bignum code. If I try running objtool -s x86/generic/bignum_mul.o I get:

x86/generic/bignum_mul.o: warning: objtool: bignum_mul() is missing an ELF size annotation

That happens on many (all?) functions. A further 28 have errors like this (presumably they are those with embedded function calls).

x86/curve25519/bignum_sqrt_p25519.o: warning: objtool: .text+0x89: unannotated intra-function call

I'm still not quite sure about whether I'm using the appropriate options nor how to interpret the output, but one encouraging difference is that the s2n-bignum mainline functions (not this CFI-enabled branch) generate zillions of errors with pretty much all instructions considered unreachable. So the CFI annotations have made a difference of some sort :-)

[torben-hansen]

Btw, this change is currently not workable in AWS-LC's FIPS build mode. The delocator is choking.

As a quick sniff test. I took the following files, copied them into AWS-LC:

• x86_att/curve25519/edwards25519_scalarmulbase.S, and
• include/_internal_s2n_bignum_x86_att.h.

I then ran a typical FIPS build:

$ cmake3 -GNinja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=$(pwd)/../awslc-install -DBUILD_SHARED_LIBS=OFF -DFIPS=1 ..
$ ninja-build
[...]
[2/242] Generating bcm-delocated.S                                                                                           
FAILED: crypto/fipsmodule/bcm-delocated.S /home/ec2-user/humble/aws-lc/awslc-build/crypto/fipsmodule/bcm-delocated.S         
[...]                                                              
error while parsing "curve25519/edwards25519_scalarmulbase.S.S": 
parse error near WS (line 47 symbol 24 - line 47 symbol 25):
"  .cfi_adjust_cfa_offset ((15*32 +8))\n\n\n\n    "

I don't think it will be terribly hard to support, but it needs to be added. The PEG parser already contains some directive argument support:

Directive <- '.' DirectiveName (WS Args)?
DirectiveName <- [[A-Z0-9_]]+
[...[
Args <- Arg ((WS? ',' WS?) Arg)*
Arg <- QuotedArg / [[0-9a-z%+\-*_@.]]*

[aqjune-aws]

I wonder whether 'canonicalizing' an .S file by (1) compiling it into .o using an assembler and (2) disassembling it through objdump and (3) using the disassembled file is supposed to pass the delocator.
I suppose delocator is failing when one is using a kind of exotic syntax; if delocator is supposed to accept syntax printed from objdump, we can reduce the complexity of this problem.

[briansmith]

Btw, this change is currently not workable in AWS-LC's FIPS build mode.

See also https://issues.chromium.org/issues/42290158.

[briansmith]

x86/generic/bignum_mul.o: warning: objtool: bignum_mul() is missing an ELF size annotation

Look at this fragment of BoringSSL assembly code:

.globl	Whatever
.type	Whatever,\@abi-omnipotent
.align	16
Whatever:
.cfi_startproc
	_CET_ENDBR
	ret
.cfi_endproc
.size	Whatever, .-Whatever

It looks like your assembly is missing the .size line. . means "the current address" so ".-Whatever" means "the difference between the current address and the address of the Whatever label," i.e. the size of the function. OpenSSL had these .size directives even before it added CFI.

Probably you want to create a macro similar to the Linux kernel's SYM_FUNC_END(name) macro that is used at the end of every function, which would do this .size directive and other boilerplate.

[briansmith]

x86/curve25519/bignum_sqrt_p25519.o: warning: objtool: .text+0x89: unannotated intra-function call

You can see how somebody fixed it here, in the kernel: https://lore.kernel.org/linuxppc-dev/[email protected]/. Basically they used the SYM_FUNC_START_LOCAL(name) macro at the start of the function and SYM_FUNC_END(name) at the end. Those two macros expand to some CFI-related metadata.

[torben-hansen]

I wonder whether 'canonicalizing' an .S file by (1) compiling it into .o using an assembler and (2) disassembling it through objdump and (3) using the disassembled file is supposed to pass the delocator. I suppose delocator is failing when one is using a kind of exotic syntax; if delocator is supposed to accept syntax printed from objdump, we can reduce the complexity of this problem.

Maybe(?) But then you assume objdump is available. I don't think this is too hard to fix. Probably just have to extend PEG to understand the argument.

[jargh]

Thanks for the interpretation of the objtool errors and the advice on fixing them; I'll give that a try.

I've not been successful in finding an objtool that works for aarch64 - any ideas there? I did find this but it seems to be work in progress:https://lwn.net/Articles/893115/. if nothing else I can use the errors that pop up on x86 as guidance for analogous things on ARM.

[briansmith]

I am a bit confused here. Is the idea that bignum_cmul_p25519 and bignum_cmul_p25519_alt are the same function, i.e. one is an alias for the other? I wonder if this pattern is allowed by all ABIs and/or if it might be the source of some of the issues here. In OpenSSL when they have such aliases, the alias is implemented as a whole separate function that jmps to the function it is aliasing.

[jargh]

We were initially trying to support a uniform naming scheme for both ARM and x86. The bifurcation into "alt" and "non-alt" arises when for at least one architecture there is value in different variants with different microarchitectural targets. Cases like this where the same function has both names reflect those where on the other architecture (only) there is value in two versions, but on this architecture it didn't seem worth it. Currently, consumers like AWS-LC handle the dispatch according to uarch. Now, we no longer quite maintain the uniformity of this naming scheme since there are a few extra functions for ARM, so perhaps we should revisit this.

[briansmith]

Why CFI_RET is used instead of just keeping normal ret and then defining a CFI_END or similar?

Why combine ret and .cfi_endproc with a macro but not .cfi_startproc and _CET_ENDBR?

[jargh]

I was attracted to wrapping things up into a unified single pseudo-instruction on the grounds that it perhaps makes it less likely that code and annotations will get out of step. I agree the current approach is not quite consistent in how it does it, though; as for _CET_ENDBR, that arose in a different PR and only applies to x86, so I'd not been thinking of it in combination, but indeed it could make sense to unify it as an "all the initial boilerplate for a function" pseudo-instruction. One slight difference is that I'm also using .cfi_startproc at internal subroutines, but those don't have the ENDBR decorations since they are only the target of internal direct calls. But I may have to think again about what CFI annotations are appropriate for internal subroutines anyway since that was one of the things that objtool was complaining about.

[briansmith]

I noticed much of the SM2 code uses rbp as a GP register whereas almost none of the other code seems to? I didn't look at the SM2 code but I suspect that wherever RBP is changed, more work needs to be done. [Edit: nevermind.]

[briansmith]

Here is one of the few places outside of SM2-specific code where RBP is changed, AFAICT.

[jargh]

Actually I think there are many more RBP changes, since very often it's hidden under macros. If you look at the objdump output and MAYCHANGE fields in the proof, you can see them all. Try for example:
grep define x86/*/*.S | grep rbp$
In particular, it's often used as a zero for ADCX/ADOX which do not allow immediate arguments.

[briansmith]

Indeed, I think I overlooked the other uses because it is referenced as ebp elsewhere, presumably as an optimization (e.g. xor ebp, ebp).

In any case, I seems like I was misremembering, since CFI will use rsp as the default base instead of rbp like I thought, so AFAICT we don't have to pay any special attention to rbp.

[jargh]

Ah yes indeed, there is also the ebp optimization. This codebase started with NASM, which makes a few optimizations such as xor ebp, ebp for you automatically if you write rbp everywhere. But we made it explicit when switching to the GNU assembler (or its clang counterpart).

[briansmith]

Do we need these function call wrapper macros? it seems like they don't do anything interesting?

[jargh]

No, that's true - I must have thought there would be some annotation to add later. I am still wondering if I might need something to pacify objtool, as suggested in its documentation, but perhaps the real issue is that the subroutine itself isn't properly annotated or is not considered as a distinct function from a CFI point of view (as was, provisionally at least, intended). I'll have to look more at the pointers you sent above.

11. file.o: warning: unannotated intra-function call

    This warning means that a direct call is done to a destination which
    is not at the beginning of a function. If this is a legit call, you
    can remove this warning by putting the ANNOTATE_INTRA_FUNCTION_CALL
    directive right before the call.