Page MenuHomePhabricator

Add cet.h for writing CET-enabled assembly code
ClosedPublic

Authored by xiangzhangllvm on May 8 2020, 12:50 AM.

Details

Summary

Add x86 feature with IBT and/or SHSTK bits to ELF program property if they are enabled. Otherwise, contents in this header file are unused.
This file is mainly design for assembly source code which want to enable CET

Diff Detail

Event Timeline

xiangzhangllvm created this revision.May 8 2020, 12:50 AM
hjl.tools added inline comments.May 8 2020, 4:04 AM
clang/test/CodeGen/asm-cet.S
2

Please replace -DIBT -DSHSTK with -fcf-protection and also test i386-pc-linux.

hjl.tools added inline comments.May 8 2020, 4:09 AM
clang/lib/Headers/cet.h
18

Please also define _CET_ENDBR. It should be for endbr64/endbr32 when IBT is enabled
and empty when IBT is disabled.

23

Please check CET instead of IBT and SHSTK.

xiangzhangllvm marked 3 inline comments as done.May 8 2020, 5:59 PM
xiangzhangllvm added inline comments.
clang/lib/Headers/cet.h
18

Hello, H.J., I not much understand here,
if CET can instead of IBT and SHSTK, why we need extra define _CET_ENDBR ?

23

So here you mean: ?
-fcf-protection --> define macro "CET" 0x3 internally in compiler;
-fcf-protection=ibt --> define macro "CET" 0x1 internally in compiler;
-fcf-protection=shstk --> define macro "CET" 0x2 internally in compiler;

clang/test/CodeGen/asm-cet.S
2

You mean use "-fcf-protection" to define macro "CET" internally in compiler?

xiangzhangllvm marked an inline comment as done.May 8 2020, 6:07 PM
xiangzhangllvm added inline comments.
clang/lib/Headers/cet.h
18

Oh, I understand it, you want define _CET_ENDBR for endbr64/endbr32, let user to use it when he/she write the assembly code.

hjl.tools added inline comments.May 8 2020, 6:48 PM
clang/lib/Headers/cet.h
18

User can use _CET_ENDBR unconditionally. You should a CET enabled assembly
codes to understand how <cet.h> is used.

23

Yes.

clang/test/CodeGen/asm-cet.S
2

Yes,

6

Add a function label and use _CET_ENDBR. You should check endbr32/endbr64.
You should assemble the same testcase without -fcf-protection. There should be
no endbr32/endbr64 nor CET marker.

xiangzhangllvm added a reviewer: pengfei.

Good thing is that: CET is already defined to 1/2/3 according to -fcf-protection=xxx at front end, So we directly used it for preprocessor.

xiangzhangllvm marked 5 inline comments as done.May 9 2020, 1:13 AM

I want to put this patch to llvm10.0.1, could you help review and then accept this patch?

hjl.tools added inline comments.May 9 2020, 4:17 AM
clang/lib/Headers/cet.h
16

Move it after __CET__ block and define it only if it isn't defined.

LuoYuanke added inline comments.May 9 2020, 7:17 AM
clang/test/CodeGen/asm-cet.S
17

This seems not test anything. Maybe test as below code.
NOENDBR-NOT: endbr

xiangzhangllvm marked 2 inline comments as done.May 10 2020, 5:23 PM
xiangzhangllvm added inline comments.
clang/lib/Headers/cet.h
16

I think It's better outside of CET block, because user can write the _CET_ENDBR in their assembly code in any case (CET enable or disable).
Or it will fail to compile if user write the _CET_ENDBR in *.S when he/she not use -fcf-protection to enable CET.

clang/test/CodeGen/asm-cet.S
17

Oh! yes, sorry for this mistake.

hjl.tools added inline comments.May 10 2020, 7:07 PM
clang/lib/Headers/cet.h
16

You have

#define _CET_ENDBR
...
#define _CET_ENDBR endbr64

Don't clang complain like

x.c:2:9: warning: '_CET_ENDBR' macro redefined [-Wmacro-redefined]
#define _CET_ENDBR endbr64
        ^
x.c:1:9: note: previous definition is here
#define _CET_ENDBR
        ^
1 warning generated.
xiangzhangllvm marked an inline comment as done.May 10 2020, 7:20 PM
xiangzhangllvm added inline comments.
clang/lib/Headers/cet.h
16

Yes, I tested it directly use the llvm-lit before, so I miss the warning information.

I add "#ifndef _CET_ENDBR" this time now.

xiangzhangllvm marked an inline comment as done.May 10 2020, 8:06 PM
xiangzhangllvm added inline comments.
clang/lib/Headers/cet.h
16

I test it, It do not have warning.
But gcc will have warning.

Add #undef _CET_ENDBR

xiangzhangllvm marked an inline comment as done.May 10 2020, 8:19 PM
xiangzhangllvm added inline comments.
clang/lib/Headers/cet.h
16

I retest it, It really has warning!! Add "#undef xxx" now.

LuoYuanke added inline comments.May 10 2020, 8:24 PM
clang/lib/Headers/cet.h
16

Should we remove line 15? If _CET_ENDBR is defined in another file, we should report warning.

hjl.tools added inline comments.May 10 2020, 8:57 PM
clang/lib/Headers/cet.h
16

I think you should do

#ifdef __CET__
...
#define _CET_ENDBR
...
#endif

#ifndef _CET_ENDBR
#define _CET_ENDBR
#endif

If _CET_ENDBR is defined by programmer, there will be a warnng,

Refine the Macro _CET_ENDBR

xiangzhangllvm marked 3 inline comments as done.May 11 2020, 12:56 AM

Refine the Macro _CET_ENDBR

Works for me.

! In D79617#2029228, @hjl.tools wrote:
Works for me.

Hello, H.J., could you help accept this patch, I hope it can go to llvm 10.0.1.
Thank you!!

Richard, should review this, since he raised some questions on the bug report.

xiangzhangllvm added a comment.EditedMay 12 2020, 5:25 PM

Hello @rsmith if you feel OK too, could you help accept it. Thank you!
Hello every friends, I plan to commit it in these days, if you do not oppose it, thank you!

This revision is now accepted and ready to land.May 18 2020, 7:01 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2020, 8:04 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tstellar reopened this revision.May 18 2020, 9:04 PM

Did @rsmith ever approve this patch? I was following the discussion at llvm.org/PR45484 but did not see an explicit approval.

This revision is now accepted and ready to land.May 18 2020, 9:04 PM
This revision was automatically updated to reflect the committed changes.

I would like a specification for this header to be added somewhere. We shouldn't be implementing random things with no specification. (Suppose someone claims that our <cet.h> is wrong in some way. How would we know whether they're right?)

Ideally, I'd also like this header to be installed somewhere where we look for assembler-with-cpp preprocessing but not for regular compilation; it doesn't make sense to me to pollute the header namespace for all C and C++ compilations with a header that is not meaningful in C and C++. But knowing whether that change is correct depends on having, you know, a specification.

I don't think we should be porting this to Clang 10. It seems clear that this is a new feature, not a bug fix.

I would like a specification for this header to be added somewhere. We shouldn't be implementing random things with no specification. (Suppose someone claims that our <cet.h> is wrong in some way. How would we know whether they're right?)

Ideally, I'd also like this header to be installed somewhere where we look for assembler-with-cpp preprocessing but not for regular compilation; it doesn't make sense to me to pollute the header namespace for all C and C++ compilations with a header that is not meaningful in C and C++. But knowing whether that change is correct depends on having, you know, a specification.

<cet.h> from GCC is as close as you can get for a reference implementation/specification.

Hello rsmith,
first, very sorry for have committed this patch before your reply, I waited 10 days, I thought you have agreed it.
I think the linux-ABI can be the specification of this head file. The context of this cet.h is according to the linux ABI about CET.
We explained in which case we should use this cet.h file. (line 2-4)
tks