Simo Sorce simo@redhat.com writes:
the attached patches have been used to successfully enable and test Intel CET support in an Intel emulator on SDV hardware.
Thanks.
GCC already has all the needed support to create CET hardened code, however the hand-coded assembly needs to be changed to conform. Without these changes all the binaries that load nettle will otherwise have CET disabled, as it is an all-or-nothing at the binary level and missing ENDBRANCH instruction cause the program to terminate on indirect jump/call instructions.
For the .note.gnu.property thing (which is per object file, right?), I think it's better to do it in the same place as ASM_MARK_NOEXEC_STACK. That's substituted in config.m4.in, and added at the end of each asm file using m4 divert.
The endbr instructions in the prologue are in the right place, as far as I can tell. And I'd be very tempted to rename the macro CET_ENDBR to COME_FROM, see https://en.wikipedia.org/wiki/COMEFROM ;-)
The second patch is used to make the system happy when hardening flags are enabled in gcc, as it generates the appropriate section information that tells the linker all is good.
I'd like to understand what's missing. Maybe we can fix it more explicitly?
Finally while looking at the assembly I noticed that some functions have a PROLOGUE() defined but not an EPILOGUE() macro defined in their .asm files. It is unclear to me if this is an error or intentional so didn't touch those, it doesn't affect functionality for this patch anyway.
I think it's an error. Except in the files in arm/fat with lines like
dnl PROLOGUE(_nettle_chacha_core) picked up by configure
I find three offending files, using
$ diff <(git grep -c '^ *EPILOGUE') <(git grep -c '^ *PROLOGUE') 49c49 < x86_64/poly1305-internal.asm:2 ---
x86_64/poly1305-internal.asm:3
51a52,53
x86_64/serpent-decrypt.asm:1 x86_64/serpent-encrypt.asm:1
have you seen any others?
Some minor comments below.
From de1b9bfeb4f8ad9a6bf8608c4b8c727dba315982 Mon Sep 17 00:00:00 2001 From: Simo Sorce simo@redhat.com Date: Tue, 23 Apr 2019 18:03:35 -0400 Subject: [PATCH 1/2] Add Intel CET protection support
In upcoming processors Intel will make available Control-Flow Enforcement Technology, which is comprised of two hardware countermeasures against ROP based attacks.
Please spell out ROP. It would be good with a link to further information in some reasonable place in the code.
The first is called Shadow Stack and checks that return from function calls are not tampered with by keeping a shadow stack that cannot be modified by aplications. This measure requires no code changes (except for code that intentionally modifes the return pointer on the stack).
Fix spelling of "aplication", "modifes"
The second is called Indirect Branch Tracking and is used to insure only targets of indirect jumps are actually jumped to. This requires modification of code to insert a special instruction that identifies a valid indirect jump target. When enforcement is turned on, if an indirect jump does not end on this special instruction the cpu raises an exception. These instructions are noops on older CPU models so it is safe to use them in all x86(_64) code.
To enable these protections gcc also inroduces a new GNU property note
and "inroduces"
+dnl GNU properties section to enable CET protections +define(<GNU_CET_SECTION>, +<ifelse(CET_PROTECTION,yes, +<.pushsection .note.gnu.property,"a"
As I said, prefer to dot his globally with an m4 divert in config.m4.in.
+ALIGN(8) +.long 1f - 0f +.long 4f - 1f +.long 5 +0: +.string "GNU" +1: +ALIGN(8) +.long 0xc0000002 +.long 3f - 2f +2: +.long 0x03 +3: +ALIGN(8) +4:
Are there no symbolic names for this note? Since we require assembler to suport endbr instructions, can we require that it know about these notes as well? What does it look like in gcc output?
diff --git a/x86_64/aes-decrypt-internal.asm b/x86_64/aes-decrypt-internal.asm index 43f2f394..5db39868 100644 --- a/x86_64/aes-decrypt-internal.asm +++ b/x86_64/aes-decrypt-internal.asm @@ -62,7 +62,7 @@ C work. define(<TMP>,<%rbp>)
.file "aes-decrypt-internal.asm"
- C _aes_decrypt(unsigned rounds, const uint32_t *keys, C const struct aes_table *T, C size_t length, uint8_t *dst,
@@ -70,6 +70,7 @@ define(<TMP>,<%rbp>) .text ALIGN(16) PROLOGUE(_nettle_aes_decrypt)
Please trim unrelated whitespace changes from the patch (I known it's not 100% consistent, but if we ever want to do something like M-x whitespace-cleanup on all files, that should be in a separate commit. On new changes, I usually run git diff --check to catch odd whitespace use).
Regards, /Niels