[PATCH] lib: sbi: expected trap must always clear MPRV

[Radim Krčmář]

2025-11-25T11:17:29-08:00, Deepak Gupta <debug at rivosinc.com>:
> On Tue, Nov 25, 2025 at 07:51:34PM +0100, Radim Krčmář wrote:
>>2025-11-25T10:03:12-08:00, Deepak Gupta <debug at rivosinc.com>:
>>> On Tue, Nov 25, 2025 at 12:12:11PM +0100, Radim Krčmář wrote:
>>>>2025-11-24T14:03:39-08:00, Deepak Gupta <debug at rivosinc.com>:
>>>>> Expected trap must always clear MPRV. Currently it doesn't. There is a
>>>>> security issue here where if firmware was doing ld/st with MPRV=1 and
>>>>> since there would be a expected trap, opensbi will continue to run as
>>>>> MPRV=1. Security impact is DoS where opensbi will just keep trapping.
>>>>
>>>>Does the DoS happen on some implementation?
>>>
>>> I ran into it while doing something else. So it was result of basically
>>> eyeballing. Didn't observe on real system.
>>>
>>>>
>>>>The expected trap came from M-mode, therefore will have mstatus.MPP=3,
>>>>so MPRV=1 should behave the same as MPRV=0.
>>>
>>> Yeah I missed that part. You have a point here.
>>>
>>> However if we read priv spec
>>> "21.4.1. Machine Status (mstatus and mstatush) Registers"
>>>
>>> ...
>>> The MPV bit (Machine Previous Virtualization Mode) is written by the
>>> implementation whenever a trap is taken into M-mode. Just as the MPP
>>> field is set to the (nominal) privilege mode at the time of the trap,
>>> ...
>>>
>>> Above text seems to suggest that nominal privilege at time of trap is
>>> set in MPP.
>>>
>>> And then just a few paragraph below if we read,
>>>
>>> ...
>>> When MPRV=1, explicit memory accesses are translated and protected,
>>> and endianness is applied, as though the current virtualization mode
>>> were set to MPV and the current nominal privilege mode were set to MPP
>>> ...
>>
>>I think that MPRV doesn't change the nominal privilege mode.
>>MPRV just modifies explicit memory accesses to behave "as through" the
>>nominal privilege mode was MPP.
>>
>>e.g. load instruction fetched with M-mode implicit access (nominal
>>privilege) performs non-M-mode explicit load (effective privilege).
>>
>>(The architecture would be broken otherwise.)
>
> Yeah I understand that's the desired behavior.
> Although current patch is additional safety and that too in not very perf
> critical path.
>
> Do you see any issue with additional safety part in the patch?

All code is an issue, and I don't see a real benefit to balance it out,
but I think it's acceptable if opensbi maintainers like the idea.

> I can modify the commit message to remove security impact (that it seems like
> how implementations are implementing it) and re-send it.

That would help.  The patch also uses a wrong bitmask for csrc:
MPRV is bit 17, but you're clearing bit 5, SPIE.
(Isn't it possible to use MSTATUS_MPRV there?)

Thanks.