Add proper CET support

[siddhesh]

Use cet.h if available and use _CET_ENDBR to make all the functions IBT-safe. This is not directly testable yet, but the instruction is in the nop space for processors that don't support it, so it's a harmless addition.

cet.h also includes the SHSTK and IBT GNU properties in ELF, so remove the manually added ones.

---

Sorry, I missed this in my previous patch because there's no Linux kernel support for IBT yet. This is a more complete fix to making libsodium properly CET-enabled.

[jedisct1]

Thanks!

I think we don't need a specific header for that; it could fit into private/common.h.

Do functions from fe51_mul et al need to have this as well? Considering the fact that they are called only from assembly code?

The previous PR only added IBT prologues to entry points. Shouldn't that be enough?

[jedisct1]

fatal error: 'cet.h' file not found

CI doesn't like it. We can have clang >= 11 without that header.

[jedisct1]

And OpenBSD has support for IBT, has <cet.h>, but not. _CET_ENDBR:

test.c:5:5: error: use of undeclared identifier '_CET_ENDBR'
    _CET_ENDBR;
    ^
1 error generated.

[jedisct1]

Well, OpenBSD does, but only when __CET__ is defined. The PR should check for __CET__ definition, or else it breaks compilation when -fcf-protection is not used.

[jedisct1]

Actually, <cet.h> has:

#ifdef __ASSEMBLER__

#ifndef __CET__
# define _CET_ENDBR
#endif

So, could the reason it is failing on OpenBSD be that __ASSEMBLER__ is not defined?

[jedisct1]

... or just that I was dumb and didn't use it in a .S file :)

[siddhesh]

Thanks!

I think we don't need a specific header for that; it could fit into private/common.h.

Ack, I'll fix this.

Do functions from fe51_mul et al need to have this as well? Considering the fact that they are called only from assembly code?

Since it's internal-only, it's not strictly needed right now, but I added it in as a precaution in case there's ever an indirect call in future.

The previous PR only added IBT prologues to entry points. Shouldn't that be enough?

The previous PR added property notes, no IBT prologues, or maybe I misunderstand your comment?

[jedisct1]

Actually zig cc does ship with cet.h, and it perfectly works in C files (where it's useless). But the path to clang headers is not set when compiling .S files.

The joy of compiler diversity :)

[siddhesh]

OK, if this is too painful across compilers then maybe it makes sense to just drop it, especially if these functions are always going to be internal-only and will likely never be called through an indirect branch. The property notes I had added in the earlier PR are sufficient to get support working.

[jedisct1]

Yes, these functions are always going to be internal, and the symbols are all private.

CET is not a Linux-only thing. It's already enforced everywhere by default in OpenBSD.

Is what you previously sent guaranteed to be compatible with all operating systems and linkers? Sorry, I'm not too familiar with this, and don't fully understand where these values come from. Is this also compatible both with _LP32 and _LP64?

[jedisct1]

Including <cet.h> still seems like the safest way to go.

I think we should just check for its existence, that is actually contains _CET_ENDBR in case of name collision, and verify that everything's fine on a bunch of compilers.

[siddhesh]

Including <cet.h> still seems like the safest way to go.

I think we should just check for its existence, that is actually contains _CET_ENDBR in case of name collision, and verify that everything's fine on a bunch of compilers.

Agreed, I tested #1291 and it works well. Closing this one, thanks!