riscv / riscv-CMOs

https://jira.riscv.org/browse/RVG-59
Creative Commons Attribution 4.0 International
77 stars 12 forks source link

Feedback on the 1.0-rc2 specification #44

Closed azuepke closed 1 month ago

azuepke commented 2 years ago

Dear authors,

here's some feedback to the 1.0-rc2 specification from a microkernel developer's point of view. Thank's for the nice work so far!

  1. Do the CBO instructions affect instruction caches?

After reading the current specification, it is not clear to me (and probably other readers as well) if the CBO instructions target both data and instruction caches, or affect data caches only. From the wording of the document ("... or instruction fetch is permitted ..."), it seems that CBO instructions could be used to invalidate instruction caches as well. Also, a dedicated an IC IVAU (ARM) or ICBI (PPC) instruction is not specified either (probably redundant to FENCE.I?). This should be clarified in the document.

  1. Mandatory write permissions for CBO.INVAL

My second concern is about potential security issues in CBO.INVAL. The specification should require mandatory write permissions in the TLBs for CBO.INVAL.

The problem is as follows: Assume a shared memory segment (SHM) between different processes. A writer process writes data to the SHM, and multiple readers read from the SHM. The readers just have read permissions to access the SHM. Now, a reader performs a CBO.INVAL on data in the SHM, as the specification only requires that:

A cache-block management 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.

However, this allows the reader to alter data in the SHM if the data is dirty in the caches. Requiring write permissions in the TLB for CBO.INVAL to succeed instead of just any of RWX permissions would fix this issue. This check should also be performed in the CBIE=01 setting when CBO.INVAL just flushes data. Always checking the write permissions makes CBO.INVAL similar to CBO.ZERO. In any case, for exceptions, reporting CBO.INVAL (and CBO.ZERO) as store page faults is fine.

Note that "being careful" and "disabling CBO.INVAL for user space and only use cache invalidation in supervisor mode" does not help here. Consider an example where the OS kernel invalidates cachelines in a mapping that is known to be writable on one core, and in parallel another core maps a different read-only page to the same address. Then the first core will continue invalidating the now read-only page. I know that this is not a realistic use case, but think of the Dirty COW exploit on Linux or VM escapes that could be prevented by checking for write permissions.

Also note that the same issue exist for the DCIMVAC instruction in the ARM v7 specification. This was fixed in ARM v8 so that DC IVAC now requires write permissions to the memory. The related DCBI instruction in PowerPC also requires write permissions. And both architectures report permission errors as failing store exceptions.

  1. CBO.CLEAN and CBO.FLUSH should report "load"-type exceptions

Related to the previous one: if CBO.CLEAN and CBO.FLUSH require just read permissions to work, then they should report a "load"-type pagefault / access fault in exceptions as well.

This helps the paging algorithm in the OS kernel. when an application performs a CBO.FLUSH to a currently unmapped (paged out) address, it would be reported as a read fault and not a write fault, so the OS could install the mapping with just read permissions for the CBO.FLUSH to succeed.

Note that this is consistent with the mental model that CBO.CLEAN and CBO.FLUSH do not alter any data, and therefore require only read permission. Also, PowerPC and ARM handle these as reads rather than writes.

I hope it's not too late to address the last two points in the specification.

All the best Alex

dkruckemyer-ventana commented 2 years ago

Thanks for your feedback. I have some short responses and some longer responses, and unfortunately, at this time, I can only address the feedback that requires short responses. But I promise to get back to you on the rest in the future!

1. Do the CBO instructions affect instruction caches?

It depends. The specification requires that

if a coherent agent in the set executes a CBO instruction that specifies the cache block, the resulting operation shall apply to any and all of the copies in the caches that can be accessed by the load and store operations from the coherent agents.

The spec also has the following non-normative comment:

An operation from a CBO instruction is defined to operate only on the copies of a cache block that are cached in the caches accessible by the explicit memory accesses performed by the set of coherent agents. This includes copies of a cache block in caches that are accessed only indirectly by load and store operations, e.g. coherent instruction caches.

So, for example, if a load could be serviced from an instruction cache via coherence mechanisms or if a store could invalidate an instruction cache, because that cache was coherent with respect to those loads and stores, then a CBO must operate on that cache. The permissions for CBOs include instruction fetch for this reason, but the specification (and the architecture) does not distinguish types of caches.

Crucially, these CBOs are only intended to operate on copies that can be accessed by explicit memory accesses, and any synchronization between implicit instruction fetches and explicit loads and stores will be defined in a forthcoming I/D consistency extension. That extension will define more fine-grained operations to manage those types of memory accesses beyond FENCE.I.

2. Mandatory write permissions for CBO.INVAL

This was a hotly debated topic within the TG as well as with the arch review committee. I plan to provide a long non-normative section in the specification to capture the salient points of the discussion and the resolution. My apologies that I cannot do justice to this topic right now.

3. CBO.CLEAN and CBO.FLUSH should report "load"-type exceptions

Again, there was a lot of discussion around this one, and it probably deserves non-normative commentary for posterity as well. At this point, I will say that there's a distinction between what permissions are required for each operation based on intended use-cases and what exceptions are reported based expected implementations. I promise I will circle back with more information.

Thanks again for your feedback!