On Mon, 2020-03-09 at 14:31 -0700, H.J. Lu wrote:
On Mon, Mar 9, 2020 at 2:15 PM Simo Sorce simo@redhat.com wrote:
On Mon, 2020-03-09 at 12:46 -0700, H.J. Lu wrote:
On Mon, Mar 9, 2020 at 12:22 PM Simo Sorce simo@redhat.com wrote:
On Mon, 2020-03-09 at 15:19 -0400, Simo Sorce wrote:
On Mon, 2020-03-09 at 11:56 -0700, H.J. Lu wrote:
On Mon, Mar 9, 2020 at 11:19 AM Simo Sorce simo@redhat.com wrote: > On Mon, 2020-03-09 at 19:03 +0100, Niels Möller wrote: > > Simo Sorce simo@redhat.com writes: > > > > > The patchset i solder than I did remember, April 2019 > > > But I recall running at least one version of it on our CET emulator @ > > > Red Hat. > > > > Sorry I forgot to followup on that. It seems only the first easy cleanup > > patch, "Add missing EPILOGUEs in assembly files", was applied back then. > > > > Do you remember why you used GNU_CET_SECTION() explicitly in .asm files, > > rather than using an m4 divert? > > Not really I do not recall anymore, but I think there was a reason, as > I recall you made that comment back then and it "didn't work out" when > I tried is the memory I have of it. > Might have to do with differences in how it lays out the code when done > via m4 divert, but not 100% sure. >
m4 divert requires much less changes. Here is the updated patch with ASM_X86_ENDBR, ASM_X86_MARK_CET_ALIGN and ASM_X86_MARK_CET.
Two comments on your patch.
- It is an error to align based on architecture. All GNU Notes MUST be
aligned 8 bytes. Since 2018 GNU Libc ignores misaligned notes.
Ah nevermind this point, misunderstanding with my libc expert, the 4 bytes alignment is ok on 32 bit code.
- It is better to use .pushsection .popsection pairs around the note
instead of .section because of the side effects of using .section
Done.
The m4 divert looks smaller impact, feel free to lift the Gnu Note section in my patch #3 and place it into your patch if you want. My code also made it more explicit what all the sections values actually mean which will help in long term maintenance if someone else need to change anything (like for example changing to enable only ShadowStack vs IBT).
Since CET support requires all objects are marked for CET, CET marker on assembly sources is controlled by compiler options, not by configure option. Also linker can merge multiple .note.gnu.property sections in a single input file:
[hjl@gnu-cfl-1 tmp]$ cat p.s .pushsection ".note.gnu.property", "a" .p2align 3 .long 1f - 0f .long 4f - 1f .long 5 0: .asciz "GNU" 1: .p2align 3 .long 0xc0000002 .long 3f - 2f 2: .long 1 3: .p2align 3 4: .popsection .pushsection ".note.gnu.property", "a" .p2align 3 .long 1f - 0f .long 4f - 1f .long 5 0: .asciz "GNU" 1: .p2align 3 .long 0xc0000002 .long 3f - 2f 2: .long 2 3: .p2align 3 4: .popsection [hjl@gnu-cfl-1 tmp]$ as -o p.o p.s -mx86-used-note=no [hjl@gnu-cfl-1 tmp]$ readelf -n p.o
Displaying notes found in: .note.gnu.property Owner Data size Description GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0 Properties: x86 feature: IBT GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0 Properties: x86 feature: SHSTK [hjl@gnu-cfl-1 tmp]$ ld -r p.o [hjl@gnu-cfl-1 tmp]$ readelf -n a.out
Displaying notes found in: .note.gnu.property Owner Data size Description GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0 Properties: x86 feature: IBT, SHSTK [hjl@gnu-cfl-1 tmp]$
New properties can be added without changing CET marker.
Here is the updated patch.
This patch looks good to me. Unfortunately I never received the original email creating the thred, did you send other patches too ?
This is the only patch needed to enable CET.
Or is the prologue stuff sufficient to pass test suite in CET emulator?
It is sufficient to pass all tests on real CET processors with
$ CC="gcc -Wl,-z,cet-report=error -fcf-protection" CXX="g++ -Wl,-z,cet-report=error -fcf-protection" /home/hjl/work/git/gitlab/nettle/configure
Excellent!
I wonder if we should create a negative test with an actual incorrect jmp instruction to verify that CET is enabled, given it is very easy to get the linker to disable CET if any dependency is not CET enabled.
When I ran the code in the emulator I manually added a jump to a non endbr instruction to make sure it would fail, and it did :-)