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

Re-integrate Smepmp #1879

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

Re-integrate Smepmp #1879

wants to merge 1 commit into from

Conversation

mickflemm
Copy link

  • Current chapter is the proposal document as-is, clean it up to better fit with the rest of the document.

  • Add a link to the proposal document for more info.

  • Add references from/to mseccfg definition, and to the PMP chapter. While there also add a reference to the Smmpm chapter on mseccfg definition.

  • Rename MMWP to MMAP (Allowlist instead of Whitelist) and update the register field definition and the visual representation fo Smepmp.

  • Clarify the reset state of mseccfg.MML/MMAP/RLB in the reset chapter.

  • Add the editors of Smepmp (me and Joe Xie) to the list of contributors.

* Current chapter is the proposal document as-is, clean it up to
better fit with the rest of the document.

* Add a link to the proposal document for more info.

* Add references from/to mseccfg definition, and to the PMP
chapter. While there also add a reference to the Smmpm chapter on
mseccfg definition.

* Rename MMWP to MMAP (Allowlist instead of Whitelist) and update
the register field definition and the visual representation fo Smepmp.

* Clarify the reset state of mseccfg.MML/MMAP/RLB in the reset chapter.

* Add the editors of Smepmp (me and Joe Xie) to the list of contributors.

Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
Copy link
Contributor

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

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

I think I was imagining a lighter touch rewording that would be a bit easier to review. Not saying the new version isn't better, it's just a bit hard to tell if everything the old version said is still present.

The definitions of the RLB, MMWP, and MML fields are furnished by the
PMP-enhancement extension, Smepmp.
The definitions of the RLB, MMAP, and MML fields are part of the Smepmp
extension, as defined in <<smepmp>>.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just hyperlink "the Smepmp extension".

Suggested change
extension, as defined in <<smepmp>>.
The RLB, MMWP, and MML fields are defined in <<smepmp,the PMP-enhancement extension, Smepmp>>.


The definition of the PMM field is furnished by the Smmpm extension.
The definition of the PMM field is furnished by the Smmpm extension in <<Zpm>>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The definition of the PMM field is furnished by the Smmpm extension in <<Zpm>>.
The PMM field is defined in <<Zpm,the Smmpm extension>>.

the platform mandates a different reset value for some PMP registers’ A
and L fields. If the hypervisor extension is implemented, the
the reset. Writable PMP registers’ A and L fields, and `mseccfg`.MML/MMAP/RLB
fields are set to 0, unless the platform mandates a pre-defined PMP ruleset.
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs a changelog entry?

@@ -2,73 +2,36 @@
== "Smepmp" Extension for PMP Enhancements for memory access and execution prevention in Machine mode, Version 1.0
=== Introduction

Being able to access the memory of a process running at a high privileged execution mode, such as the Supervisor or Machine mode, from a lower privileged mode such as the User mode, introduces an obvious attack vector since it allows for an attacker to perform privilege escalation, and tamper with the code and/or data of that process. A less obvious attack vector exists when the reverse happens, in which case an attacker instead of tampering with code and/or data that belong to a high-privileged process, can tamper with the memory of an unprivileged / less-privileged process and trick the high-privileged process to use or execute it.
The Smepmp extension enhances the xref:pmp[xrefstyle=basic] mechanism to improve Machine mode's security, by providing stronger memory isolation and privilege separation. It enables preventing the execution of code from memory regions not explicitly marked as executable by Machine mode, and restricts Machine mode's access to memory regions designated for Supervisor and User modes. These capabilities help to mitigate control-flow subversion attacks that exploit execution of unauthorized code with Machine mode privileges, and provide security guarantees consistent with the mechanism described in <<sum>> for memory access and execution prevention between Supervisor and User mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Smepmp extension enhances the xref:pmp[xrefstyle=basic] mechanism to improve Machine mode's security, by providing stronger memory isolation and privilege separation. It enables preventing the execution of code from memory regions not explicitly marked as executable by Machine mode, and restricts Machine mode's access to memory regions designated for Supervisor and User modes. These capabilities help to mitigate control-flow subversion attacks that exploit execution of unauthorized code with Machine mode privileges, and provide security guarantees consistent with the mechanism described in <<sum>> for memory access and execution prevention between Supervisor and User mode.
The Smepmp extension enhances the xref:pmp[xrefstyle=basic] mechanism to improve Machine mode's security by providing stronger memory isolation and privilege separation. It allows preventing the execution of code from memory regions not explicitly marked as executable by Machine mode, and restricts Machine mode's access to memory regions designated for Supervisor and User modes. These capabilities help to mitigate control-flow subversion attacks that exploit execution of unauthorized code with Machine mode privileges, and provide security guarantees consistent with the mechanism described in <<sum>> for memory access and execution prevention between Supervisor and User mode.

Tbh I think the old text was a lot clearer. I would just change the second paragraph to

To prevent this attack vector, two mechanisms known as Supervisor Memory Access Prevention (SMAP) and Supervisor Memory Execution Prevention (SMEP) can be used. The first one prevents the OS from accessing the memory of an unprivileged process unless a specific code path is followed, and the second one prevents the OS from executing the memory of an unprivileged process at all times. RISC-V includes support for SMAP, through the sstatus.SUM bit, and for SMEP by always denying execution of virtual memory pages marked with the U bit, with Supervisor mode (OS) privileges, as mandated on the Privilege Spec. This extension adds support for similar mechanisms for Machine mode.

Copy link
Author

Choose a reason for hiding this comment

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

It makes more sense to explain what this extension does than why it's required, the why makes more sense in the proposal doc, than in the privilege spec. No information is lost here, We still summarize the attack vector, and reference the smap/smep behavior on S-mode for consistency, and it's more compact than the previous version. Also reading the original text people may assume that there is also a specific code path in this case, when instead we use shared regions for that purpose, so the more we mention what S-mode does, the more we risk misleading people.


However, there are no such mechanisms available on Machine mode in the current (v1.11) Privileged Spec. It is not possible for a PMP rule to be *enforced* only on non-Machine modes and *denied* on Machine mode, to only allow access to a memory region by less-privileged modes. it is only possible to have a *locked* rule that will be *enforced* on all modes, or a rule that will be *enforced* on non-Machine modes and be *ignored* by Machine mode. So for any physical memory region which is not protected with a Locked rule, Machine mode has unlimited access, including the ability to execute it.

Without being able to protect less-privileged modes from Machine mode, it is not possible to prevent the mentioned attack vector. This becomes even more important for RISC-V than on other architectures, since implementations are allowed where a hart only has Machine and User modes available, so the whole OS will run on Machine mode instead of the non-existent Supervisor mode. In such implementations the attack surface is greatly increased, and the same kind of attacks performed on Supervisor mode and mitigated through SMAP/SMEP, can be performed on Machine mode without any available mitigations. Even on implementations with Supervisor mode present attacks are still possible against the Firmware and/or the Secure Monitor running on Machine mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is all useful text! I don't think it should be deleted; just reworded.

Choose a reason for hiding this comment

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

imo this is all redundant. it reads like a textbook, not an ISA standard

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why an ISA standard shouldn't have some motivation. There are plenty of notes already in the specification that aren't normative.

It could be condensed though for sure. I think it's at least worth mentioning that this is most useful for M/U machines where supervisor protection mechanisms cannot be used.

Copy link
Author

Choose a reason for hiding this comment

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

Protecting Machine mode is always relevant, in any implementation scenario. What we said in the original document was that on systems where the OS runs on Machine mode, the attack surface is greatly increased, not because you don't have S-mode protections in-between, but because you run more complex code on M-mode with a higher probability of a vulnerability being present there that would allow for the exploitation of memory accesses from M-mode to less privileged modes ( tricking M-mode to execute code from less privileged modes). So it's not that Smepmp is most useful for M/U machines, we make this clear in the original document, in the last sentence of the part you quoted "Even on implementations with Supervisor mode present attacks are still possible...". With CoVE for example coming up, where we are going to have more complex code running on M-mode, it makes perfect sense to have Smepmp available, OpenSBI (which targets systems with Supervisor mode available) already uses it.

In any case this further explains the "why" not the "what" or "how", IMHO it makes more sense to keep the extended version of the "why" part in the proposal document which is referenced in case anyone wants to dig deeper, than having it as part of the ISA manual. The short version is still there: "to improve Machine mode's security by providing stronger memory isolation and privilege separation". I agree non-normative text still makes sense which is why I added tips e.g. for "how" shared regions may be used, or "why" the last two restrictions were added to MML, I think it's cleaner to have short tips than whole paragraphs.

[[proposal]]
=== Proposal

. *Machine Security Configuration (mseccfg)* is a new RW Machine mode CSR, used for configuring various security mechanisms present on the hart, and only accessible to Machine mode. It is 64 bits wide, and is at address *0x747 on RV64* and *0x747 (low 32bits), 0x757 (high 32bits) on RV32*. All mseccfg fields defined on this proposal are WARL, and the remaining bits are reserved for future standard use and should always read zero. The reset value of mseccfg is implementation-specific, otherwise if backwards compatibility is a requirement it should reset to zero on hard reset.
Copy link
Contributor

Choose a reason for hiding this comment

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

All mseccfg fields defined on this proposal are WARL

This seems to have been lost.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch ! We make it clear than all bits are locked either on set or on clear, but we should say they are WARL in any case, I'll update the pull request.


The ``mseccfg.RLB`` bit provides a mechanism to temporarily modify Locked PMP rules:

* When mseccfg.RLB=1, Locked PMP rules may be modified and locked PMP entries may be edited.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* When mseccfg.RLB=1, Locked PMP rules may be modified and locked PMP entries may be edited.
* When mseccfg.RLB=1 *locked* PMP rules may be modified and *locked* PMP entries may be edited.


.. If ``mseccfg.MML`` is not set, the combination of ``pmpcfg.RW=01`` remains reserved for future standard use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been lost?

Copy link
Author

Choose a reason for hiding this comment

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

It seemed redundant, we only override this when MML is set, didn't make much sense to say that when MML is not set, those bits remain reserved. It's also clean from the visual representation, but if you think it's worth re-adding it, I'm ok with it.

.. In case ``mseccfg.MMWP`` is not set, M-mode can still access and execute any region not covered by a PMP rule. Since we try to prevent M-mode from executing malicious code and since an attacker may manage to place code on some region not covered by PMP (e.g. a directly-addressable flash memory), we need to ensure that M-mode can only execute the code segments initialized during firmware / OS initialization.
=== Smepmp software discovery

Since all fields defined on ``mseccfg`` under this proposal are either locked when set (``MMAP``/``MML``) or locked when cleared (``RLB``), software cannot dynamically query them to determine the presence of Smepmp. It is expected that BootROM will set ``mseccfg.MMAP`` and/or ``mseccfg.MML`` during early boot, before transferring control to the firmware. The firmware can then determine the presence of Smepmp by reading ``mseccfg`` and verifying the state of ``mseccfg.MMAP`` and ``mseccfg.MML``. Alternatively a software-defined discovery mechanism may be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

BootROM -> "the firmware"?

Copy link
Author

Choose a reason for hiding this comment

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

"BootROM, ...during early boot, before transferring control to the firmware" The idea here is that the BootROM is implementation-specific, while the firmware as a software component may be more implementation-agnostic, such as OpenSBI for example or another third-party firmware (it makes perfect sense to have a reference firmware implementation like OpenSBI, that we can maintain/verify etc, and use that in most situations).


[[rationale]]
=== Rationale
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like a shame to lose all this information too.

Copy link
Author

Choose a reason for hiding this comment

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

I think the important stuff is still there, some as part of the implementation considerations, some as tips/info, and the most important part regarding rlb is mentioned twice. It seems inconsistent with the rest of the document to keep this, it's a bit too much to e.g. explain why we didn't say anything about mprv, or what was our thought process etc. The document is still there for people to dig deeper, it doesn't have to be included in the privilege spec. The chapter was 7 pages long, it's now 4 pages long and still contains what's needed for someone to implement this.

@gfavor
Copy link
Collaborator

gfavor commented Mar 3, 2025 via email

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