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

[Deepak Gupta]

On Tue, Nov 25, 2025 at 08:48:35PM +0100, Radim Krčmář wrote:
>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'll wait for Atish and Anup then.

>
>> 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?)

It's `lui` (load upper immediate). I could just do `li a4, MTSTATUS_PRV`.
Although its a larger immediate, so most likely assembler will split into
two instructions or would replace it with `lui`.

>
>Thanks.