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

Allowing delayed check for jump/branch instructions #23

Closed
kliuMsft opened this issue Jan 9, 2024 · 15 comments
Closed

Allowing delayed check for jump/branch instructions #23

kliuMsft opened this issue Jan 9, 2024 · 15 comments

Comments

@kliuMsft
Copy link
Contributor

kliuMsft commented Jan 9, 2024

In the case of branch/jump instructions, would like to have the option to delay the PCC-based branch target checks (address bound/permission) to instruction fetch time (errors associated with the branch target instruction), rather than handling it at EX time (error associated with the branch/jump themselves).

The benefit is better performance (checking logic will not be in the critical instruction feth timing path, for example) and fewer changes to execution logic (espically branch instructions).

@rmn30
Copy link
Collaborator

rmn30 commented Jan 9, 2024

I think I agree that delaying the bounds check until instruction fetch of the target PC makes sense, even though it potentially makes debugging harder. In many cases the link register will indicate the offending instruction, so hopefully this isn't too big a problem (certainly no worse the before CHERI!). The major exception to this is returns (cret) which will mostly target sealed capabilities. Can we keep the tagged and unsealed checks on the jump / branch? Other jumps / branches without link are likely to have immediate offsets which are much less likely to go out of bounds if linking and PCC are correct.

Note that removing the bounds check will also solve #18 .

@rmn30
Copy link
Collaborator

rmn30 commented Jan 9, 2024

Note that @vmurali has in mind a JIT compiler / dynamic linker where trap at the destination may not be sufficient.

@kliuMsft
Copy link
Contributor Author

kliuMsft commented Jan 9, 2024

@rmn30 for register-targeted jumps (jr/jalr) I don't see any issues with doing tag/unseal checks at EX time. The performance concerns are mostly with address bound checking. So we can,

  • For c.JR/JALR, keep the tag/unsealed checking but defer address bound checking to fetching
  • For all other PCC-based jump/branches (conditioanl branching and JAL), defer all checks to fetching

Does this sound right?

@vmurali
Copy link
Collaborator

vmurali commented Jan 9, 2024

One has to store the previous PC in case of taken branches/jumps in MEPCC (or have a different architectural register storing the previous PC) as suggested in #18, to make debugging simpler.

@vmurali
Copy link
Collaborator

vmurali commented Jan 28, 2024

I posted a comment on #18. Basically JALRs, JALs and Branches would now need a representability check :(.

@vmurali
Copy link
Collaborator

vmurali commented Feb 17, 2024

EDIT: I wrote a long comment with an alternative design, but realized that the alternative won't work. But I want to reiterate that we can't avoid checking for either bounds or representability for jumps/branches in execute.

rmn30 pushed a commit that referenced this issue Feb 19, 2024
…d MTCC behaviour.

As per #23 we want to delay the bounds check on jumps / branches until instruction
fetch in order to simplify hardware. Due to this potentially leading to
unrepresentable MEPCC values we also clear the tag of MEPCC on instruction fetch
bounds violations. Due to this this got a bit mixed up with #30 which clarifies
and simplifies validation and legalization of MEPCC / MTCC.
rmn30 pushed a commit that referenced this issue Feb 19, 2024
…d MTCC behaviour.

As per #23 we want to delay the bounds check on jumps / branches until instruction
fetch in order to simplify hardware. Due to this potentially leading to
unrepresentable MEPCC values we also clear the tag of MEPCC on instruction fetch
bounds violations. Due to this this got a bit mixed up with #30 which clarifies
and simplifies validation and legalization of MEPCC / MTCC.
rmn30 pushed a commit that referenced this issue Feb 19, 2024
…d MTCC behaviour.

As per #23 we want to delay the bounds check on jumps / branches until instruction
fetch in order to simplify hardware. Due to this potentially leading to
unrepresentable MEPCC values we also clear the tag of MEPCC on instruction fetch
bounds violations. Due to this this got a bit mixed up with #30 which clarifies
and simplifies validation and legalization of MEPCC / MTCC.
rmn30 pushed a commit that referenced this issue Feb 23, 2024
…d MTCC behaviour.

As per #23 we want to delay the bounds check on jumps / branches until instruction
fetch in order to simplify hardware. Due to this potentially leading to
unrepresentable MEPCC values we also clear the tag of MEPCC on instruction fetch
bounds violations. Due to this this got a bit mixed up with #30 which clarifies
and simplifies validation and legalization of MEPCC / MTCC.
@rmn30
Copy link
Collaborator

rmn30 commented Feb 29, 2024

Closing due to #37 being merged.

@rmn30 rmn30 closed this as completed Feb 29, 2024
@vmurali
Copy link
Collaborator

vmurali commented Feb 29, 2024

I am not sure how just clearing the tag bit in MEPCC is sufficient when a Branch/JAL/JALR target is unrepresentable (especially Branch and JAL). It could be unrepresentable in such a way that the bounds check with the new address would pass during instruction fetch.

If the tag for MEPCC is cleared, how would one recover from Instruction Bounds Violation exception? That is, when I run MRET after handling this exception (say by changing the address to point to the previous instruction which I just rewrite in the trap handler), how will it go back to that compartment/code block?

@vmurali
Copy link
Collaborator

vmurali commented Feb 29, 2024

If you really want to push all of Branch/JAL/JALR bound checks into fetch, the correct way to do it is to save the previous PCC in the case of Branch and JAL, and perform a bounds check based on the previous PCC; in case of JALR, save the target PCC in the previous PCC register and perform a bounds check based on that register.

And instead of complicating the behavior of MEPCC by clearing the tag, you can just store the previous PCC in a new SCR (MEPrevPCC). Write/Read legalization is orthogonal to this change (and I agree that clearing tag whenever legalization occurs is a simpler and sufficient design choice).

@davidchisnall
Copy link
Collaborator

I am not sure how just clearing the tag bit in MEPCC is sufficient when a Branch/JAL/JALR target is unrepresentable (especially Branch and JAL). It could be unrepresentable in such a way that the bounds check with the new address would pass during instruction fetch.

The bounds check will succeed, but the tag check will not.

If the tag for MEPCC is cleared, how would one recover from Instruction Bounds Violation exception? That is, when I run MRET after handling this exception (say by changing the address to point to the previous instruction which I just rewrite in the trap handler), how will it go back to that compartment/code block?

Currently, this kind of error will always invoke the error handler or force unwind. The error handler receives an untagged copy of the PCC (to preserve the control-flow-integrity properties of libraries and any secrecy that they enforce built atop those properties) and the switcher will rederive it form the compartment's PCC.

@rmn30
Copy link
Collaborator

rmn30 commented Feb 29, 2024

There's an implicit assumption that the PCC bounds are stored expanded, so the representability issue only arises when we take the bounds exception on instruction fetch and need to construct something to put in MEPCC. In the Sail PC and PCC are separate and PCC never has its address set. The arch doc could make this clearer.

@rmn30
Copy link
Collaborator

rmn30 commented Feb 29, 2024

The bounds check will succeed, but the tag check will not.

Not quite. It's still a bounds failure on instruction fetch. It's only MEPCC that has tag cleared.

@vmurali
Copy link
Collaborator

vmurali commented Feb 29, 2024

Currently, this kind of error will always invoke the error handler or force unwind. The error handler receives an untagged copy of the PCC (to preserve the control-flow-integrity properties of libraries and any secrecy that they enforce built atop those properties) and the switcher will rederive it form the compartment's PCC.

Okay, this looks fine.

There's an implicit assumption that the PCC bounds are stored expanded, so the representability issue only arises when we take the bounds exception on instruction fetch.

Can you clarify this in the ISA doc? Storing PCC bounds as expanded is part of the spec now. The spec should also state that JAL and Branch do not affect the expanded bounds/expanded meta cap data. It should only change the address. JALR should install a new expanded meta cap data.

@davidchisnall
Copy link
Collaborator

Storing PCC bounds as expanded is part of the spec now.

I think we need to be a bit careful here. I don’t want to require implementations to keep the bounds expanded, but I don’t mind if they need to act as if the bounds are expanded and handle PC-relative jumps that lead to unrepresentable values in a consistent and well-defined way.

@vmurali
Copy link
Collaborator

vmurali commented Feb 29, 2024

I think we need to be a bit careful here. I don’t want to require implementations to keep the bounds expanded, but I don’t mind if they need to act as if the bounds are expanded and handle PC-relative jumps that lead to unrepresentable values in a consistent and well-defined way.

If they are not stored expanded, the new PCC may not have any bearing to the previous PCC w.r.t. representability. A more complicated spec would be what I had outlined - compare the new PCC's address on JAL and Branch with the previous PCC's bounds. I believe both are equivalent (and storing the previous PCC in the spec is more complex in my opinion).

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

No branches or pull requests

4 participants