Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options for enable code gen with CFI `-fcf-protection=[full|branc… #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kito-cheng
Copy link
Collaborator

@kito-cheng kito-cheng commented Aug 16, 2024

…h|return|none]and-mcf-label-scheme=[unlabeled|func-sig]`

Resue the options defined by X86 CET, -fcf-protection=[full|branch|return|none]

-fcf-protection=branch for landing pad (Zicfilp), -fcf-protection=return for landing pad (Zicfiss) and -fcf-protection=full for enable both if possible, landing pad just require instrcution defined by base extension, so compiler will emit landing pad even without Zicfilp extension, but -fcf-protection=return will require at least Zimop since the instrcution isn't included in base extension.

Also we defined another option for specify the labeling scheme: unlabeled and func-sig.

The unlabeled scheme is always use lpad 0, and func-sig is based on the function signature, the rule is defined in psABI.


Currently clang/LLVM using -fsanitize=shadow-call-stack to control the shadow stack, also that shared same option with software shadow shadow, so I think we have three options for this:

  1. Let -fsanitize=shadow-call-stack alias to -fcf-protection=return
  2. Let -fsanitize=shadow-call-stack only for software shadow stack
  3. Drop -fcf-protection=return and only -fsanitize=shadow-call-stack

I am OK with option 1 or 2 and prefer option 2, and dislike option 3 since it means we have very different flavor option naming scheme on landing pad and shadows stack...

@mylai-mtk
Copy link

I believe all mentions of the simple scheme would be changed to unlabeled in the future?

README.mkd Outdated Show resolved Hide resolved
README.mkd Outdated Show resolved Hide resolved
@kito-cheng
Copy link
Collaborator Author

I believe all mentions of the simple scheme would be changed to unlabeled in the future?

Oh, yeah, apparently I lost my memory when I wrote this PR

[1] riscv-non-isa/riscv-elf-psabi-doc#417 (comment)

…h|return|none]` and `-mcf-label-scheme=[unlabeled|func-sig]`

Resue the options defined by X86 CET, `-fcf-protection=[full|branch|return|none]`

`-fcf-protection=branch` for landing pad (`Zicfilp`), `-fcf-protection=return`
for landing pad (`Zicfiss`) and `-fcf-protection=full` for enable both
if possible, landing pad just require instrcution defined by base
extension, so compiler will emit landing pad even without `Zicfilp`
extension, but `-fcf-protection=return` will require at least `Zimop`
since the instrcution isn't included in base extension.

Also we defined another option for specify the labeling scheme: `unlabeled`
and `func-sig`.

The `unlabeled` scheme is always use `lpad 0`, and `func-sig` is based
on the function signature, the rule is defined in psABI.
@kito-cheng
Copy link
Collaborator Author

Changes:

  • Rename -mcf-label-scheme= to -mcf-branch-label-scheme=.
  • Fix typo
  • Update to adoc format

@jaidTw
Copy link

jaidTw commented Sep 18, 2024

Do we have conclusion on which option to implement now?

@mylai-mtk
Copy link

mylai-mtk commented Sep 19, 2024

Do we have conclusion on which option to implement now?

I'm good with -fcf-protection=branch|full and -fcf-branch-label-scheme=unlabeled|func-sig. I tried implementing it in Clang, and currently there's no problem. (I picked -fcf-branch-label-scheme=func-sig as default for Zicfilp :P)

Besides the adoption of this -fcf-protection=branch flag, I want to ask if we're going implement the [no]cf_check attribute as introduced in x86 Function Attributes?

- `none`: Disable control flow protection.
- `full`: Protect all control flow instructions, will enable branch protection
and return protection if the `Zimop` extension is available.
- `branch`: Protect branch instructions only by insert landing pad.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insert -> inserting

`-fcf-protection` is alias to `-fcf-protection=full`.

- `none`: Disable control flow protection.
- `full`: Protect all control flow instructions, will enable branch protection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence reads like branch protection requires Zimop, but I think only return protection requires Zimop?

@quic-garvgupt
Copy link

Hi @kito-cheng,

To clarify our last discussion during the RISCV sync-up, does -fcf-protection=return now imply -fsanitize=shadow-call-stack? Additionally, according to the documentation, -fcf-protection=return requires the Zimop extension to be enabled. If this extension is not enabled, will a software shadow stack be generated instead?

@kito-cheng
Copy link
Collaborator Author

kito-cheng commented Sep 30, 2024

@quic-garvgupt

At LLVM sync up meeting, we would like use -fsanitize=shadow-call-stack for software shadow stack, and -fcf-protection=return for hardware shadow stack only, one option control one feature, so no worry about zimop disturb -fsanitize=shadow-call-stack :)

@quic-garvgupt
Copy link

Thank you, @kito-cheng, for your response. I have one more clarification: when referring to -fsanitize=shadow-call-shadow, do we actually mean -fsanitize=shadow-call-stack? I was unable to find any option named -fsanitize=shadow-call-shadow, and Clang returns an “unsupported option” error. Is this a new option that will be supported in the future, or is it simply a typographical error?

@kito-cheng
Copy link
Collaborator Author

kito-cheng commented Sep 30, 2024

@quic-garvgupt oh, that's my stupid typo (and copy/paste), should be -fsanitize=shadow-call-stack, thanks for point out that, let me fix :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants