Removed that prefetch instructions can throw exceptions #7
Conversation
|
@francislaus : Thanks for PR. This issue with prefetches was discussed extensively and it was decided that it is best to prevent software from arbitrarily execute prefetch instructions without at least some minimum requirements on the authorising capability. The rationale for this is to reduce the attack surface through prefetches. The discussion is summarised in this issue from the original CHERI technical report: CTSRD-CHERI/cheri-specification#65 I agree that the prerequisites need fixing! |
|
@andresag01 Thank you for your comment. I think there might have been a misunderstanding. The PR does not intend on allowing prefetches to execute if the authorising capability does not allow. I think the way to go in this case is to make them NOPs as suggested by Jessica. However, what my PR intends on doing is to not throw exceptions in this case. That is exactly what the (Zicboc) RISC-V spec does. It does not allow access to the cache block, but it does also not raise exceptions. I think this would be sensible because pre-fetch instructions are HINTs and can be safely ignored. I think this PR is in line with the discussion you mentioned as well as the Zicboc extension. It would probably be useful to explain the reasoning about this in the document. Do you think it would be sensible to add a few words explaining that? |
|
@andresag01 Yes, I think the discussion, and the thrust of the conclusion in that issue, was to include the metadata to allow hardware to do checks and avoid side channels, not to throw exceptions. In the last reply to that thread, you said: "Do not start the prefetch if any of the checks above fails." Prefetch instructions aren't useful if they can throw exceptions. |
|
@francislaus @PeterRugg : Ah right! Sorry, I misunderstood your comments (and my own meeting minutes! 🤦). In that case, the change looks good, but it would be good to clarify in the text that the refetch is not supposed to go ahead if certain checks fail -- I do not think the current text makes that clear or spells out the checks to be done. What do you think? |
|
Yes, @francislaus probably has comments due to his work on side channels, but we should have a section discussing what is allowed to happen even for traditional loads and stores. It doesn't (traditionally) belong in the architecture to discuss the allowed effects on microarchitectural state, but it seems appropriate to have some text, at least as a note, indicating that memory accesses where the checks fail should not escape to the memory hierarchy. It is particularly important for prefetches though because an attacker would be able to prefetch without having to squash the exception, presumably increasing the bandwidth considerably. |
|
Agreed. The checks that we previously discussed are listed in the table that is being deleted; clearly these must not raise exceptions though. It would be good to keep a record of that somewhere in the text even if its a microarchitectural comment, which the RISC-V specs do have in some places. |
|
I agree 100% that prefetches must not throw exceptions, this was a mistake in the specification. |
|
@francislaus we should conditions under which execution the prefetch executes as a NOP (tag clear, sealed, no bytes in bounds in the cache line). Can you add this text? |
|
@tariqkurd-repo Sure, I am just working on it. |
…ch instructions can be considered as NOPs
|
@tariqkurd-repo It is all ready for the final approval now. I have added the conditions for the prefetch to not be issued as well as a very short intro text for our rationale on that. Credit to @PeterRugg for feedback. |
|
@francislaus : Thanks for the change. It looks good in my humble opinion. One minor comment I would make is that here:
It might not be clear what the word "issue" means at the ISA level. I would tweak this to say something like the memory access is not "performed"... |
|
@andresag01 Sounds sensible to me. |
src/insns/prefetch.i.adoc
Outdated
| * The tag is not set | ||
| * The sealed bit is set | ||
| * No bytes of the cache line requested is in bounds | ||
| * The X permission is not set |
There was a problem hiding this comment.
Should be <<x_perm>> to get the cross reference
src/insns/prefetch.r.adoc
Outdated
| * The tag is not set | ||
| * The sealed bit is set | ||
| * No bytes of the cache line requested is in bounds | ||
| * The R permission is not set |
There was a problem hiding this comment.
Should be <<r_perm>> to get the cross reference
src/insns/prefetch.w.adoc
Outdated
| * The tag is not set | ||
| * The sealed bit is set | ||
| * No bytes of the cache line requested is in bounds | ||
| * The W permission is not set |
There was a problem hiding this comment.
Should be <<w_perm>> to get the cross reference
I removed that prefetch instructions can throw exceptions. This is in line with the Zicbop rationale on prefetch instructions:
"A cache-block prefetch instruction is permitted to access the specified cache block whenever a load instruction,
store instruction, or instruction fetch is permitted to access the corresponding physical addresses. If access to the
cache block is not permitted, a cache-block prefetch instruction does not raise any exceptions and shall not
access any caches or memory. During address translation, the instruction does not check the accessed and dirty
bits and neither raises an exception nor sets the bits."
Furthermore, I added Zicbop as a prerequisite for the two
prefetch.winstructions.