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

Legalise underivable caps #36

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

Legalise underivable caps #36

wants to merge 1 commit into from

Conversation

PeterRugg
Copy link
Contributor

Tested to match the Bluespec cheri-cap-lib implementation (PR pending).

Also includes refactoring to allow an intermediate struct without any decoding. This is required to neatly clear the IE bit, but has the side-effect of reducing duplicated code in preludes.

@arichardson
Copy link
Member

I'm not sure how useful it is to share struct Capability between 64 and 128. We might provide a subset of permissions for 64 in the future? But I guess we could always hardcode the others to false.

Also this PR needs to be rebased.

@PeterRugg
Copy link
Contributor Author

I'm not sure how useful it is to share struct Capability between 64 and 128. We might provide a subset of permissions for 64 in the future? But I guess we could always hardcode the others to false.

Also this PR needs to be rebased.

Hmm, yes, or factor out the permissions into a separate struct in that case. It seemed silly to duplicate all the decoding logic at least...

I'll do a rebase now. It seems to mostly be trivial mantissa_width vs cap_mantissa_width.

@PeterRugg PeterRugg force-pushed the underivable branch 2 times, most recently from 080b6e0 to 69869a4 Compare October 20, 2020 12:55
Copy link
Contributor

@bauereiss bauereiss left a comment

Choose a reason for hiding this comment

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

Looks good to me, I've just left a few syntactic suggestions. The properties I've been SMT-checking before still hold, and I was able to strengthen the "(base <= top)" property by removing the precondition that the capability has canonical bounds, which is good.

src/cheri_cap_common.sail Outdated Show resolved Hide resolved
src/cheri_cap_common.sail Outdated Show resolved Hide resolved
Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

Seems fine to me, I'll re-generate the sail->C and fuzz against cheri-compressed-cap to see how long it takes libfuzzer to find a mismatch.

src/cheri_cap_common.sail Outdated Show resolved Hide resolved
src/cheri_cap_common.sail Outdated Show resolved Hide resolved
@PeterRugg
Copy link
Contributor Author

Seems fine to me, I'll re-generate the sail->C and fuzz against cheri-compressed-cap to see how long it takes libfuzzer to find a mismatch.

Sounds good, although we do expect differences: it might take some effort to differentiate bugs from intended changes. I haven't looked at the cheri-compressed-cap code much, but could add the changes to that if needed.

@PeterRugg
Copy link
Contributor Author

@jrtc27 It would be nice to get this merged before too much rebasing is required. Any chance you're going to have time to take a look at it in the near future?

@PeterRugg
Copy link
Contributor Author

I've tidied up the history and pulled most of the uncontroversial contents into master.

let correction_top = tHi - aHi in {
base_corrected : CapLenBits = base + (to_bits(cap_len_width, correction_base) << (cap_mantissa_width + E));
top_corrected : CapLenBits = top + (to_bits(cap_len_width, correction_top) << (cap_mantissa_width + E));
/* Interpret the top relative to the address space of the base.
Copy link
Member

Choose a reason for hiding this comment

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

Could you expand on this comment, e.g. explaining why this prevents base > top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't think this alone prevents that. It's more ensuring that the top hasn't wrapped around to be in an entire address space (or more) below the base? I must admit, my memory is slightly hazy.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's been a while since I looked at this which is why I wasn't sure why we can now assert that base <= top. Adding some comments explaining where this is guaranteed would be helpful for future readers I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, agreed. I guess "it's been proved" is something, but it's also nice to see how it's been constructed to make this true.

src/cheri_cap_common.sail Show resolved Hide resolved
arichardson added a commit to arichardson/cheri-compressed-cap that referenced this pull request Jan 13, 2023
Due to the way we decode top/bot it is possible for top to decode to
1<<66, but that is then truncated to zero and can therefore be less than
the base of the capability. The example test case was found by TestRIG

See CTSRD-CHERI/sail-cheri-riscv#36 for an
alternative decoding approach that guarantees top >= base.
@bauereiss bauereiss self-requested a review January 13, 2023 18:25
arichardson added a commit to CTSRD-CHERI/cheri-compressed-cap that referenced this pull request Feb 1, 2023
Due to the way we decode top/bot it is possible for top to decode to
1<<66, but that is then truncated to zero and can therefore be less than
the base of the capability. The example test case was found by TestRIG

See CTSRD-CHERI/sail-cheri-riscv#36 for an
alternative decoding approach that guarantees top >= base.
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.

4 participants