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

[Deepak Gupta <debug@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@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@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?
I can modify the commit message to remove security impact (that it seems like
how implementations are implementing it) and re-send it.

>
>> So if take them together, it seems like nominal priv at time trap can be
>> less than 3 and same should reflect in MPP if it gets trapped.
>
>non-M nominal privilege mode can only be reached via mret/sret, and
>mret/sret clear MPRV, so there should be no way to enter a trap handler
>with MPRV == 1 && MPP != 3.
>
>(The expected trap handler should only be configured during opensbi's
> M-mode execution anyway.)
>
>> I don't know what implementations are doing. Should ask around.
>
>I wouldn't be surprised to find bugs in those corner cases...

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi