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

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

[Deepak Gupta]

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.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 lib/sbi/sbi_expected_trap.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/sbi/sbi_expected_trap.S b/lib/sbi/sbi_expected_trap.S
index 99dede5f..8fbd2cb2 100644
--- a/lib/sbi/sbi_expected_trap.S
+++ b/lib/sbi/sbi_expected_trap.S
@@ -23,6 +23,8 @@
 	.global __sbi_expected_trap
 __sbi_expected_trap:
 	/* Without H-extension so, MTVAL2 and MTINST CSRs and GVA not available */
+	lui	a4, 0x20 /* MSTATUS_MPRV */
+	csrc	CSR_MSTATUS, a4
 	csrr	a4, CSR_MCAUSE
 	REG_S	a4, SBI_TRAP_INFO_OFFSET(cause)(a3)
 	csrr	a4, CSR_MTVAL
@@ -39,6 +41,8 @@ __sbi_expected_trap:
 	.global __sbi_expected_trap_hext
 __sbi_expected_trap_hext:
 	/* With H-extension so, MTVAL2 and MTINST CSRs and GVA available */
+	lui	a4, 0x20 /* MSTATUS_MPRV */
+	csrc	CSR_MSTATUS, a4
 	csrr	a4, CSR_MCAUSE
 	REG_S	a4, SBI_TRAP_INFO_OFFSET(cause)(a3)
 	csrr	a4, CSR_MTVAL
-- 
2.43.0


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

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

[Radim Krčmář]

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?

The expected trap came from M-mode, therefore will have mstatus.MPP=3,
so MPRV=1 should behave the same as MPRV=0.

Thanks.

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

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

[Deepak Gupta]

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
...


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.

I don't know what implementations are doing. Should ask around.

>
>Thanks.

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

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

[Radim Krčmář]

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

> 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

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

[Deepak Gupta]

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

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

[Radim Krčmář]

2025-11-25T11:17:29-08:00, 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?

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.

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

Re: [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@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?
>
>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.

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

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

[Radim Krčmář]

2025-11-25T12:04:44-08:00, Deepak Gupta <debug@rivosinc.com>:
> On Tue, Nov 25, 2025 at 08:48:35PM +0100, Radim Krčmář wrote:
>>2025-11-25T11:17:29-08:00, Deepak Gupta <debug@rivosinc.com>:
>>> 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`.

Sorry, I missed that.  Yeah, assembler should translate it to just lui.

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

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

[Anup Patel]

On Tue, Nov 25, 2025 at 3:33 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> 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.
>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>  lib/sbi/sbi_expected_trap.S | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/lib/sbi/sbi_expected_trap.S b/lib/sbi/sbi_expected_trap.S
> index 99dede5f..8fbd2cb2 100644
> --- a/lib/sbi/sbi_expected_trap.S
> +++ b/lib/sbi/sbi_expected_trap.S
> @@ -23,6 +23,8 @@
>         .global __sbi_expected_trap
>  __sbi_expected_trap:
>         /* Without H-extension so, MTVAL2 and MTINST CSRs and GVA not available */
> +       lui     a4, 0x20 /* MSTATUS_MPRV */

This should be "li a4, MSTATUS_MPRV". I will take care
of it at the time of merging this patch.

> +       csrc    CSR_MSTATUS, a4
>         csrr    a4, CSR_MCAUSE
>         REG_S   a4, SBI_TRAP_INFO_OFFSET(cause)(a3)
>         csrr    a4, CSR_MTVAL
> @@ -39,6 +41,8 @@ __sbi_expected_trap:
>         .global __sbi_expected_trap_hext
>  __sbi_expected_trap_hext:
>         /* With H-extension so, MTVAL2 and MTINST CSRs and GVA available */
> +       lui     a4, 0x20 /* MSTATUS_MPRV */

Same comment as above.

> +       csrc    CSR_MSTATUS, a4
>         csrr    a4, CSR_MCAUSE
>         REG_S   a4, SBI_TRAP_INFO_OFFSET(cause)(a3)
>         csrr    a4, CSR_MTVAL
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

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