| Message ID | 20251124220339.3695940-1-debug@rivosinc.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib: sbi: expected trap must always clear MPRV | expand |
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.
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.
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...
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...
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.
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.
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.
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
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
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(+)