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

Clarify behaviour of scss with malformed bounds #279

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Jun 4, 2024

This clarifies that malformed capabilities have bounds of [0,0) for the purposes of SCSS. I think their bounds don't affect any other instructions except GCBASE and GCLEN which already specify 0. CBLD is unaffect because it checks for malformed capabilities explicitly.

This behaviour can almost be inferred from GCBASE and GCLEN, but I think it's better to be explicit rather than to depend on the assumption that SCSS behaves the same as those instructions.

Fixes #281

This clarifies that malformed capabilities have bounds of [0,0) for the purposes of SCSS. I think their bounds don't affect any other instructions except GCBASE and GCLEN which already specify 0. CBLD is unaffect because it checks for malformed capabilities explicitly.

This behaviour can almost be inferred from GCBASE and GCLEN, but I think it's better to be explicit rather than to depend on the assumption that SCSS behaves the same as those instructions.
@PRugg-Cap
Copy link
Contributor

Agreed. You could imagine saying that SCSS always returns false if either input is malformed, but I think this way is fine too, and encourages designs to split out the legalisation early for the real bounds checks as well. Good to have an explicit note either way.

@tariqkurd-repo
Copy link
Collaborator

LGTM. @Timmmm can you file an issue to match?

Copy link
Collaborator

@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.

It shouldn't really matter what outcome you get here, but specifying a sensible behaviours makes a lot of sense to me.

@tariqkurd-repo tariqkurd-repo merged commit 0bcec0e into riscv:main Jun 5, 2024
3 checks passed
@jamie-melling
Copy link

jamie-melling commented Jun 6, 2024

late in adding this, but I think from a HW perspective it's much better to always return false when either cs1 or cs2 is malformed.
Currently, it looks like

isMalformed(cs1) isMalformed(cs2) scss
0 0 cs2.xbounds <= cs1.xbounds
0 1 true
1 0 cs2.xbounds == 0
1 1 true

It's the case that cs1 is malformed while cs2 is not which adds extra logic. In this case, we need to check that the bounds of cs2 are zero for it to have bounds that are equal or lesser. It's much easier to just say, that if the bounds are malformed it cannot be a subset - since we are arbitrarily deciding a standardization.

@Timmmm
Copy link
Contributor Author

Timmmm commented Jun 6, 2024

Do you think all hardware would be implemented like that? I mean I would have thought you just have some kind of "bounds decoder" unit and then always do the comparison, so your SCSS unit doesn't actually care about whether the bounds are malformed or not (basically how the Sail code works).

That "bounds decoder" would already output 0s for malformed caps because that's what GCBASE/GCLEN require.

(I'm not a hardware designer though so maybe it's not that simple?)

@tariqkurd-repo
Copy link
Collaborator

It's certainly simpler to say "just return zero" because that way it's just a single AND gate on the result if either input is malformed.
It's better if the spec doesnt push additional logic into the calculation, and forcing bounds to zero after calculation and before the comparison is a waste of logic delay.

So I'm in favour of simplifying it. Cheaper hardware, and a defined output.

@Timmmm
Copy link
Contributor Author

Timmmm commented Jun 6, 2024

Sounds reasonable. The only issue I can imagine is that it's a bit inconsistent with gcbase and gclen - if software wanted to calculate scss using those then it would be inconsistent. I dunno if you'd really want to do that tbf but it might be a bit of a paper cut? Presumably malformed caps are ideally rare?

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 6, 2024

How much is it really simplifying though? And I don’t think you can say it’s always simplifying for every uarch. I much prefer being consistent in the architecture to adding potential warts that may minorly improve the size of some uarches.

@PRugg-Cap
Copy link
Contributor

Eh, on the architectural consistency front this could go either way: you might expect that SCSS is consistent with CBLD, for example.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 6, 2024

That's a fair point

@tariqkurd-repo
Copy link
Collaborator

While we're at it, are there any other instructions which need additional definition for malformed inputs? It would be good to fix them all at once

@PRugg-Cap
Copy link
Contributor

Going through all the Zcheripurecap instructions:
7.1.1. CMV Either detag or leave the tag (see note below)
7.1.2. MODESW N/A: shouldn't be able to have a malformed cap as PCC
7.1.3. CADDI Should probably clear the tag, as it can't meaningfully do a representability check
7.1.4. CADD As CADDI
7.1.5. SCADDR As CADDI
7.1.6. ACPERM Doesn't touch bounds and legalises permissions, so same as CMV
7.1.7. SCMODE As CMV
7.1.8. SCHI Always clears the tag, so fine.
7.1.9. SCEQ Needs to be a proper bit-compare, so don't do anything special for malformed
7.1.10. SENTRY As CMV
7.1.11. SCSS As we've discussed here, go for "always return false" if either is malformed?
7.1.12. CBLD Can't set the tag on malformed things
7.1.13. GCTAG The current spec wording would imply this should return 0 on malformed inputs.
7.1.14. GCPERM Just extracts permissions. Gives 0 on malformed permissions
7.1.15. GCHI Needs to just extract the bits, nothing special for malformed
7.1.16. GCBASE Gives 0 on malformed bounds
7.1.17. GCLEN Gives 0 on malformed bounds
7.1.18. SCBNDSI Subset check should fail if input bounds malformed, tag needs to be cleared if producing a malformed cap, which is now possible
7.1.19. SCBNDS Same as SCBNDSI
7.1.20. SCBNDSR Same as SCBNDSI
7.1.21. CRAM Nothing relevant
7.1.22. LC Shouldn't allow loads via malformed caps, could clear the tag on loading a malformed cap.
7.1.23. SC Shouldn't allow stores via malformed caps, could clear the tag on storing a malformed cap.
From other instructions, it's just jumps that seem interesting: Shouldn't be allowed to malformed caps

It's important to make sure CBLD and SCBNDS* (plus similarly ACPERM) refuse to produce tagged malformed caps, which we've now addressed: these are the only way to actually create a tagged one architecturally. That should just leave LC as a way for one to enter the processor.

Then just SCSS, GCLEN, GCBASE, GCPERM, SCMODE have special cases that affect how they present the values other than just for the tag. I think we've addressed all of these well now.

To cover the rest of the instructions, I can then see four different reasonable strategies:

  1. Assume it never happens and don't worry about it. If memory can bit flip or get you uninitialised, potentially tagged values, you're already in trouble.
  2. Always do the malformed check whenever you're writing back a cap, anding the result with the tag. This is presumably tricky on lc in terms of timing.
  3. Always do a malformed check when reading from the register file and clear the tag input to other things if it fails. I imagine this is reasonable: the hardware has to be there anyway, and the tag shouldn't be a timing-critical input into other things. Might be bad for power though...
  4. (Slightly different to 3, might be better for power, but more confusing to get right). Do the malformed check wherever the tag can be observed only, so on GCTag, as the store data, and when being used to authorise operations, but not on CMV and friends.

Architecturally, I think 2, 3 and 4 are indistinguishable, so the spec doesn't have to clarify. 1 is observably different from the others only if you're worried about what to do if the memory can contain things that weren't written by the core.

All that to say, I don't think we need extra clarification, though the current wording of "Capabilities with malformed bounds are always invalid anywhere in the system i.e. their tags are always 0." could be read to mean 1 (i.e. it's telling you they can't exist tagged so the core doesn't have to worry about it), or (2, 3 or 4), i.e. this is something the design should make sure is true.

@tariqkurd-repo
Copy link
Collaborator

Just a quick comment, although we need to go through them all thoroughly.
Making CMV change the output prevents an out-of-order CPU from just increasing the reference count of the source physical register instead of actually copying the data, so would have performance implications.

All the others actually require execution, although modifying load data is undesirable (although we already plan to tag clear on LC on some occasions - but that's not dependant on the loaded data).

@PRugg-Cap
Copy link
Contributor

Good point! Sounds like 4. may be the way to go in practice then.

@tariqkurd-repo
Copy link
Collaborator

maybe - although that adds more data dependant logic to stores.
how about 4 but not on stores? would we want memcpy to change data?

@PRugg-Cap
Copy link
Contributor

PRugg-Cap commented Jun 6, 2024

To clarify from the meeting, maybe it makes sense to just make any bounds check fail given a malformed cap. That probably wants to be explicit (there's a weirdness that a malformed cap would let you setBounds a real cap with bounds [0, 0) if you decide to legalise it that way). To redo the list:

7.1.1. CMV Doesn't care
7.1.2. MODESW Can't happen
7.1.3. CADDI Representability check fails: clear tag
7.1.4. CADD As CADDI
7.1.5. SCADDR As CADDI
7.1.6. ACPERM Doesn't care
7.1.7. SCMODE Doesn't care
7.1.8. SCHI Always clears tag, so doesn't care
7.1.9. SCEQ Needs to be a proper bit-compare, so don't do anything special for malformed
7.1.10. SENTRY Doesn't care
7.1.11. SCSS As we've discussed here, go for "always return false" if either is malformed?
7.1.12. CBLD Can't set the tag on malformed things
7.1.13. GCTAG Happy to return 1 for malformed caps (provided they are tagged!)
7.1.14. GCPERM Just extracts permissions. Gives 0 on malformed permissions. Doesn't care about bounds
7.1.15. GCHI Needs to just extract the bits, nothing special for malformed
7.1.16. GCBASE Gives 0 on malformed bounds
7.1.17. GCLEN Gives 0 on malformed bounds
7.1.18. SCBNDSI Subset check should fail if input bounds malformed, tag needs to be cleared if producing a malformed cap, which is now possible
7.1.19. SCBNDS Same as SCBNDSI
7.1.20. SCBNDSR Same as SCBNDSI
7.1.21. CRAM Doesn't care
7.1.22. LC Authorising bounds check fails on malformed bounds. Doesn't do anything special on loading a malformed cap.
7.1.23. SC Authorising bounds check fails on malformed bounds. Doesn't do anything special on storing a malformed cap.
From other instructions, it's just jumps that seem interesting: Bounds check fails when jumping to malformed caps

That probably makes much more sense: you only do the malformed check when you're already doing the much more expensive bounds check anyway, and only things that care about the bounds check for them being a reserved encoding, just like permissions. Sorry for overcomplicating it!

@PRugg-Cap
Copy link
Contributor

"Capabilities with malformed bounds are always invalid anywhere in the system i.e. their tags are always 0." should probably be changed to something like "Bounds checks always fail if the authorising capability has malformed bounds. The core will never construct a tagged capability with malformed bounds."

@tariqkurd-repo
Copy link
Collaborator

Some thoughts:

We need to define:

  1. all bounds calculate as zero for malformed tagged caps: GCBASE, GCLEN, SCBNDS* (for SCBNDS* it has the result of detagging the result)
  2. some instructions as propagating malformed tagged caps with no modification: CMV, LC, SC
  3. some instructions as detagging malformed tagged caps: CADDI, CADD, SCADDR
  4. some instructions as propagating malformed tagged caps, even though there is a metadata update: ACPERM, SCMODE, SENTRY
  5. all get instructions other than in point 1 above don't both with the malformed check i.e. GCTAG, GCPERM, GCHI
  6. SCEQ does a full bitwise compare regardless

I think that covers it....

@Timmmm
Copy link
Contributor Author

Timmmm commented Jun 7, 2024

some instructions as detagging malformed tagged caps: CADDI, CADD, SCADDR

I think this is not necessary because the address of a malformed cap doesn't matter since its bounds are 0?

@PRugg-Cap
Copy link
Contributor

some instructions as detagging malformed tagged caps: CADDI, CADD, SCADDR

I think this is not necessary because the address of a malformed cap doesn't matter since its bounds are 0?

I suspect it will actually be cheaper to just detag: otherwise hardware will have to wait for the mux on the bounds to resolve before feeding it into the representability check?

@PRugg-Cap
Copy link
Contributor

Some thoughts:

We need to define:

  1. all bounds calculate as zero for malformed tagged caps: GCBASE, GCLEN, SCBNDS* (for SCBNDS* it has the result of detagging the result)
  2. some instructions as propagating malformed tagged caps with no modification: CMV, LC, SC
  3. some instructions as detagging malformed tagged caps: CADDI, CADD, SCADDR
  4. some instructions as propagating malformed tagged caps, even though there is a metadata update: ACPERM, SCMODE, SENTRY
  5. all get instructions other than in point 1 above don't both with the malformed check i.e. GCTAG, GCPERM, GCHI
  6. SCEQ does a full bitwise compare regardless

I think that covers it....

I agree that should be the effect. I don't think this needs to litter the spec too much though? Hopefully we can specify that malformed caps fail all bounds and representability checks, and people can look at the sail to make fully sure? The current notes on GCBASE and GCLEN plus the proposed note on SCSS should be the only cases that need to address it explicitly?

@tariqkurd-repo
Copy link
Collaborator

tariqkurd-repo commented Jun 7, 2024

I would say that the page for every instruction which writes a capability should have a malformed cap handling statement so it's unambiguous. Which will be a small number of include files, and pull the correct one into each page. I'll give that a go.....

@jamie-melling
Copy link

Just been discussing tagged malformed permissions with Tariq and what to do with them when dereferencing. Keeping in line with the GCPERM and ACPERM behaviour, if ACPERM could not have created the permissions, all permissions should be treated as 0.

For instance, a tagged cap with ASR + load + store (and no execute) will cause a permissions violation if you try to dereference it.

@PRugg-Cap
Copy link
Contributor

Yes, sorry, I was imagining that behaviour, but I guess we didn't really define it anywhere. I think the idea of malformed caps is a little confusing because it covers {malformed bounds, reserved permissions encodings, reserved bits set}. It sounds like we're converging on the behaviour that {malformed bounds => read as zero and fail all bounds checks, reserved perms => read as zero and fail all perm checks, reserved bits => don't let the tag become set (with CBLD) but otherwise ignore}

tariqkurd-repo added a commit that referenced this pull request Jun 11, 2024
More clarification on malformed capability handling, following the
discussion in #279

cmv, lc, sc, amoswap.c propagate malformed caps
caddi, cadd, scaddr, scbnds* de-tag
gchi, gcperm ignore
gcbase, gclen return 0

also if the permission can't be created by ACPERM then clear _all_
permissions, which affect load/store/cbo/amo dereferencing and also the
target capability for JALR

---------

Signed-off-by: Tariq Kurd <[email protected]>
Co-authored-by: Alexander Richardson <[email protected]>
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.

Clarify behaviour of scss with malformed bounds
6 participants