PMP and MPU Improvements #1873
PMP and MPU Improvements #1873
Conversation
alistair23
force-pushed
the
alistair/mpu-improvements
branch
from
5b1590d
to
de02960
Compare
|
I pushed an update that splits out the KernelMPU and fixes some comments. |
alistair23
force-pushed
the
alistair/mpu-improvements
branch
from
de02960
to
d7027d6
Compare
alistair23
force-pushed
the
alistair/mpu-improvements
branch
from
d7027d6
to
4ec9d2e
Compare
|
Ping! |
|
I will take a look by end of week. |
There was a problem hiding this comment.
Overall, I think this looks good. I think clear should be taken out of the trait, and the comments describing what these functions do need to be rewritten -- I think they are in a few cases saying the opposite of what they want to say. It seems like something we could easily hammer out in 30 minutes or so.
alistair23
force-pushed
the
alistair/mpu-improvements
branch
from
4ec9d2e
to
ea3ff08
Compare
|
I just pushed an update. I have tried to improve the documentation. I removed the boards clearing the MPU. I have left the Let me know what you think. |
|
Ping! |
|
Ping^2 @phil-levis or @bradjc any comments? |
|
Ping^3 |
This patch does a few things: - Convert the disable_mpu() function to clear_mpu() - Make the disable_mpu() function a NOP on ARMv7 and RV32 - Rename the enable/disable functions to be app specific Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
alistair23
force-pushed
the
alistair/mpu-improvements
branch
from
ea3ff08
to
5b0d254
Compare
|
Can this be merged? |
|
Ping again |
|
cc @phil-levis bors can't merge this until you remove your request for changes |
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
|
bors r+ |
alistair23
dismissed stale reviews from phil-levis and hudson-ayers
via
1adfde8
|
Canceled. |
|
I just pushed a patch adding the comments discussed in the OT call. Can we re-run bors? Sorry for the confusion @hudson-ayers, you were too quick :) |
|
bors r+ |
|
Hi @alistair23, Is there an issue I could follow tracking the implementation of Thanks! |
|
Hey @ia0. I am not doing anything related to Can you point to documentation that describes the issue? Can you also point to a way to reproduce it? You are saying that because we no longer do |
Yes, this is essentially what we currently do as a workaround until we can setup a kernel MPU config.
I'll try to create a "minimal" reproduction case when I get time.
I see. However, we might need some work in that direction to avoid the regression. Implementing |
|
@tock/core-wg this sounds like it could be a regression that we may want to fix before a 1.6 release? @alistair23 or @ia0, would it be possible to get the partial revert fix into a PR relatively quickly? |
|
The We could either clear the enable bit when returning to the kernel (as we did previously) or we could change the MPU permissions to only affect userspace. The second option matches what we do for RISC-V, so I think that's better. Something like this diff: diff --git a/arch/cortex-m3/src/mpu.rs b/arch/cortex-m3/src/mpu.rs
index 004d52194..729a65977 100644
--- a/arch/cortex-m3/src/mpu.rs
+++ b/arch/cortex-m3/src/mpu.rs
@@ -275,14 +275,14 @@ impl CortexMRegion {
RegionAttributes::XN::Disable,
),
mpu::Permissions::ReadExecuteOnly => {
- (RegionAttributes::AP::ReadOnly, RegionAttributes::XN::Enable)
+ (RegionAttributes::AP::UnprivilegedReadOnly, RegionAttributes::XN::Enable)
}
mpu::Permissions::ReadOnly => (
- RegionAttributes::AP::ReadOnly,
+ RegionAttributes::AP::UnprivilegedReadOnly,
RegionAttributes::XN::Disable,
),
mpu::Permissions::ExecuteOnly => {
- (RegionAttributes::AP::NoAccess, RegionAttributes::XN::Enable)
+ (RegionAttributes::AP::PrivilegedOnly, RegionAttributes::XN::Enable)
}
}; |
|
I confirm that the diff in #1873 (comment) fixes the issue. It would take some work to make a minimal or even small reproduction case. But here are the reproduction steps for the actual issue:
Address |
|
PR created: #2120 |
Commit 128782d "mpu: Change the disable_mpu API" converted the disable_app_mpu() function to not make any changes. This means when we return from an app to the kernel we don't disable the MPU. This resulted in some access failures when the kernel tried to access direct write flash regions (tock#1873 (comment)). We can either disable the MPU when returning to the kernel or change the app MPU configuration permissions to not impact the kernel. This patch converts the Cortex-M3 MPU implementation to always allow the kernel access when configuring the app. This way we can avoid the overhead of disabling the MPU on context switches. There is no security gap here as the kernel could just disable the MPU anyway if it was malicious. Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
2120: arch/cortex-m3: Allow the kernel to access protected memory r=bradjc a=alistair23 ### Pull Request Overview Commit 128782d "mpu: Change the disable_mpu API" converted the disable_app_mpu() function to not make any changes. This means when we return from an app to the kernel we don't disable the MPU. This resulted in some access failures when the kernel tried to access direct write flash regions (#1873 (comment)). We can either disable the MPU when returning to the kernel or change the app MPU configuration permissions to not impact the kernel. This patch converts the Cortex-M3 MPU implementation to always allow the kernel access when configuring the app. This way we can avoid the overhead of disabling the MPU on context switches. There is no security gap here as the kernel could just disable the MPU anyway if it was malicious. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> ### Testing Strategy None. ### TODO or Help Wanted ### Documentation Updated - [X] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [X] Ran `make prepush`. Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
Pull Request Overview
This PR replaces:
And combines a few different MPU/PMP improvements
enable/disable_mpu()functions to beenable/disable_app_mpu()functions. This is because these functions only enable and disable the MPU for apps.disable_mpu()function to beclear_mpu()and run it at boot. Instead of clearing the MPU/PMP after returning to the kernel from an app let's instead do nothing as we don't need to. We still leave the function around though, encase a future architectures wants to use it. This should improve syscall performance, especially for RISC-V. We also clear the MPU/PMP at boot encase a previous stage has set anything up.RISC-V ePMP
The RISC-V enhanced PMP spec is making progress and it seems like it is unlikely to have any major changes before ratification.
In anticipation of this let's start to prepare adding support to Tock.
This PR updates the MPU trait to allow marking regions of kernel memory with permissions.
The idea here is that before the MPU is enabled Tock will set read/write/execute permissions for itself. This will limit what Tock itself can access, for example by removing write permissions from code.
We can also add a check to see if it is already enabled. If the previous stage has enabled lock down mode we won't set it ourselves. We will have to trust the previous stage (that would have loaded us) to have done it correctly. If no one else has set it, we can set it for enhanced security.
Once enabled we can't change the execute permission (read/write can change) so let's enforce that all regions are added before enabling.
Testing Strategy
Running on QEMU, I'll test the Unleashed and Apollo3 as well.
TODO or Help Wanted
Documentation Updated
/docs, or no updates are required.Formatting
make format.make clippy.