Closed Wesdcv closed 1 month ago
Before making additional changes, on this branch, i wanted to check a couple of things with @CharlyCst.
handle_virtual_load_store()
.
https://github.com/CharlyCst/miralis/blob/36abbd7cf324f757034d70f1aa75db5bece0de17/src/arch/metal.rs#L532 However, should there be support for such a possibility in the code?fid=20
(that's a random number for MIP patching) in the codebase or if we should look at the underlying issue in Linux to see if we can get rid of the need for ecall?
https://github.com/CharlyCst/miralis/blob/36abbd7cf324f757034d70f1aa75db5bece0de17/src/virt.rs#L622-L638To give a quick answer (we can discuss more in details next week or by following up here):
MPRV=1
then the PMP take effect as if S or U mode was executing. We should double check what the spec says around that, but it seems to contradict what you observed? If PMP are taken into account we only need to worry about (3), and we already have a function to check if we touch a device (for now we can panic if that's the case, I don't think we will need it for Linux).Changes to the draft state:
idx=0
is disabled now instead of allowing RWX
, due to check priority it was allowing all the accesses)Tested with Linux payload on Spike
- works fine. The PR is not yet ready to be merged to my mind, but i would like to get some feedback now.
If PMP are taken into account we only need to worry about (3), and we already have a function to check if we touch a device (for now we can panic if that's the case, I don't think we will need it for Linux).
Yes, we check if the mtval corresponds to any memory-mapped physical address, i was asking more about if access like this could ever be a case:
┌───────┐
┌───────┐│
│ PT ││
vMPRV=1 │ ││
┌────────────────────►│ │├────────────────────────┐
│ │ ││ │
│ │ │┘ │
│ └───────┘ │
┌─────────────┴───────────┐ ┌─────────────▼───────────┐
│ │ │ │
│ ld a0, 0(0xFF...F8..) │ │ ld a0, 0(0x2000_0000) │
│ │ │ │
└─────────────────────────┘ └─────────────────────────┘
If such an access could happen, i won't be caught by https://github.com/CharlyCst/miralis/blob/9d842700c3b9ec0f637eb2110ebb6f183f79b631/src/virt.rs#L499 and subsequently will go through direct emulation instead of a device access interface.
I guess this question falls into the same basket as protecting Miralis's memory from access with MPRV=1 (which means we should somehow validate the virtual address).
Yes I think this should be caught by PMPs too... We could potentially write a test to ensure this is the case (see if the PMPs indeed prevents from reading Miralis memory or devices using MPRV).
Thanks for the PR! I'll review it when I get some time, most likely this Friday.
As this is a non-trivial feature, would you mind writing a description of how the emulation works? You can add that as a new section to the doc in ./docs/architecture.md
.
Addressed most of the issues, what's left:
handle_virtual_load_store
requires any assumptions to be safe.Ping me when you would like me to take another look :)
There is a "re-request review" button at the top right in the "Reviewers" section adds a "to-review" item in my notification list :)
Now everything should be more or less covered, the only question left is with virtual addresses validation
Regarding the virtual address validation, my intuiting and understanding is that the PMP should catch if the access is invalid
That should indeed be the case for addresses that we're purely protecting (like Miralis's memory), but if we're using PMP as a mean of catching access attempt (like with devices), an access fault would be returned to firmware instead of a correct access emulation chain. That might be an undesired behavior.
Draft PR: For now in order for this to work on VisionFive2 board a modified version of linux is required. It also breaks
sandbox
test CI.