Fix assembler call frame information (CFI) directives

[jovanbulck]

Current macros emit an ENDBR instruction between the function label and corresponding cfi_start, which confuses binary analysis tools like llvm-bolt.

Also add missing cfi_start/end directives for other assembly functions.

See also: https://sourceware.org/binutils/docs/as/CFI-directives.html

[haitaohuang]

Can you explain why it "confuses binary analysis tools like llvm-bolt." and why llvm-bolt can't be changed to handle this kind of code and you need to 'fix" code instead of tool?
Other than confusing the tool, do you see any functional/security issue?

[jovanbulck]

Thanks for following up on this. The problem is essentially that the current SDK code incorrectly uses the cfi_startproc and cfi_endproc directives as per the contract.

From the binutils link above:

.cfi_startproc is used at the beginning of each function that should have an entry in .eh_frame. It initializes some internal data structures. Don’t forget to close the function by .cfi_endproc.

There are two issues in the current SDK code that clearly violate this "contract" and that this commit fixes:

1. ENDBR is emmitted after the function name and before .cfi_startproc and thus it does not hold that ".cfi_startproc is used at the beginning of each function"
2. for some functions, there is no corresponding cfi_endproc and thus it does not hold that "Don’t forget to close the function by .cfi_endproc. "

Can you explain why it "confuses binary analysis tools like llvm-bolt."

The current SDK code does not abide by the unambiguous contract quoted above. Thus, the entries in .eh_frame. are unclosed and possibly off-by-one (ENDBR), which leads to off-by-one errors in the rewritten code by tools by BOLT.

and why llvm-bolt can't be changed to handle this kind of code and you need to 'fix" code instead of tool?

IMHO the "fix" here is clearly on the SGX SDK, as it does not abide by the unambiguous usage contract quoted above and BOLT is not to blame for clearly misannotated code. It seems infeasible and undesirable to me to fix all possible misuses of low-level asm directives in parsing tools..

Other than confusing the tool, do you see any functional/security issue?

afais there's no security issues (the ENDBR is what is used and the cfi_start/endproc is merely annoation at teh binary level), but not sure as I'm not super familiar with low-level CFI..

Hope this helps. Let me know if there are remaining questions or concerns!

[haitaohuang]

thanks. it all makes sense to me now

[haitaohuang]

Looks good to me

[haitaohuang]

You do need add Signed-off-by tag to your commit before this can be merged

[jovanbulck]

Thanks, I force-pushed to add a Signed off by tag

[lzha101]

Will consider to merge the PR after our upcoming release.