[PATCH v2 0/2] target/riscv: report the no-PMP-rule fault on the S/U fetch rather than on mret

[PATCH v2 0/2] target/riscv: report the no-PMP-rule fault on the S/U fetch rather than on mret

[tewaro]

Hi,

When PMP is implemented but no PMP entry has been configured, QEMU
raises the resulting access fault on the mret itself rather than on the
first S/U-mode fetch after it. The access is correctly denied either
way, but because the trap is taken before the privilege switch, S-mode
can never be entered on such a configuration. Patch 1 has the details
and the history; patch 2 adds a tests/tcg/riscv64 system test that
fails before and passes after it.

This is not reachable from S/U-mode or from a guest, since mret and
mnret are M-mode-only and PMP is configured from M-mode. The affected
software is firmware that executes mret with PMP present and no entry
configured, so I do not consider this a security issue. The same
symptom was reported in GitLab issue #2074; the earlier discussions in
Launchpad #1923197 and GitLab #585 were about whether the access should
fail, which is not in question here.

Tested with make check-tcg for riscv64-softmmu, and by rebuilding
v6.0.0 and v7.2.0 with only the mret check removed to confirm that the
normal PMP path has always handled this case on its own.

Provenance: the bug was found, and the analysis above was debugged, with
the help of Claude (Anthropic), used as a research and debugging aid as
permitted by docs/devel/code-provenance.rst. The code and the text of
the patches were written by me.

Changes in v2:
- Rebased onto current master (73294d096c). The new meson.build entry
  now sits among the tests added since v1; no other change.
- Resent over SMTP. v1 went out through the Gmail API, which rewrapped
  long lines, stripped trailing whitespace and replaced the Message-IDs,
  so it did not apply and the patches were not threaded under the cover
  letter. Thanks to Daniel for catching it.

v1: https://lore.kernel.org/qemu-devel/CAEq2Q90UbCK2=7SFGQErviup1=u0AYVf=n90NaJvo1M9N=_BUA@mail.gmail.com/


tewaro (2):
  target/riscv: do not raise the no-PMP-rule access fault from mret
  tests/tcg/riscv64: add test for mret to S-mode with no PMP entry

 target/riscv/tcg/op_helper.c         |  10 +--
 tests/tcg/riscv64/system/meson.build |   1 +
 tests/tcg/riscv64/test-mret-nopmp.S  | 100 +++++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 8 deletions(-)
 create mode 100644 tests/tcg/riscv64/test-mret-nopmp.S

-- 
2.54.0 (Apple Git-157)

[PATCH v2 1/2] target/riscv: do not raise the no-PMP-rule access fault from mret

[tewaro]

Consider a CPU that implements PMP but has no PMP entry configured.
If M-mode executes mret with MPP set to S or U, QEMU raises an
instruction access fault on the mret. The trap is taken in M-mode,
with mepc pointing at the mret, mtval zero and MPP equal to M.

The spec says the mret succeeds and the first S/U-mode fetch fails
[1][2]. The trap should therefore be taken from S/U-mode, with mepc
and mtval equal to the mret target [3][4] and MPP equal to S/U [5].

The early check in check_ret_from_m_mode() is what moves the fault
onto the mret. Without it, the fetch at the target misses the TB and
TLB caches, which are keyed by privilege level, and reaches
pmp_hart_has_privs_default() through riscv_cpu_tlb_fill(). That
function already denies S/U-mode when no rule matches. Nothing
executes between the privilege switch and that fetch, so no other
behaviour changes.

As it stands, S/U-mode can never be entered on such a configuration,
and a handler that steps mepc past the apparent fault resumes in
M-mode at the S-mode entry point (GitLab issue #2074).

The check was added by commit d102f19a208 alongside the pmp.c change
that makes the no-rule path M-mode-only, and I believe it was already
redundant then. Building v6.0.0 and v7.2.0 with only the check removed
confirms the fetch faults as expected. Later commits (0fbb5d2d3c9,
4c48aad1, 3157a553, 16aa7771) changed its gating, cause and location
but not the normal path. Remove it and the now unused prev_priv
parameter.

The bug was found and debugged with the help of Claude (Anthropic),
used as a research and debugging aid; the patch was written by hand.

[1] Privileged ISA v1.12, 3.7.1 "Physical Memory Protection CSRs",
    Priority and Matching Logic
[2] Privileged ISA v1.12, 3.3.2 "Trap-Return Instructions"
[3] Privileged ISA v1.12, 3.1.14 "Machine Exception Program Counter"
[4] Privileged ISA v1.12, 3.1.16 "Machine Trap Value Register"
[5] Privileged ISA v1.12, 3.1.6.1 "Privilege and Global
    Interrupt-Enable Stack in mstatus register"

Privileged ISA v1.12 (ratified, 2021-12-03):
https://github.com/riscv/riscv-isa-manual/releases/tag/Priv-v1.12
Current source, same wording:
https://github.com/riscv/riscv-isa-manual/blob/4877e41a/src/priv/machine.adoc

Fixes: d102f19a2085 ("target/riscv/pmp: Raise exception if no PMP entry is configured")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2074
Signed-off-by: tewaro <tewaro@theorem.dev>
---
v2: rebased onto current master; no change.

 target/riscv/tcg/op_helper.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/target/riscv/tcg/op_helper.c b/target/riscv/tcg/op_helper.c
index f9c79fe6b9..e46190079a 100644
--- a/target/riscv/tcg/op_helper.c
+++ b/target/riscv/tcg/op_helper.c
@@ -388,7 +388,6 @@ target_ulong HELPER(sret)(CPURISCVState *env)
 }
 
 static void check_ret_from_m_mode(CPURISCVState *env, target_ulong retpc,
-                                  privilege_mode_t prev_priv,
                                   uintptr_t ra)
 {
     if (!(env->priv >= PRV_M)) {
@@ -400,11 +399,6 @@ static void check_ret_from_m_mode(CPURISCVState *env, target_ulong retpc,
                                     env->misa_ext) && (retpc & 0x3)) {
         riscv_raise_exception(env, RISCV_EXCP_INST_ADDR_MIS, ra);
     }
-
-    if (riscv_cpu_cfg(env)->pmp &&
-        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
-        riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, ra);
-    }
 }
 static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
                                    privilege_mode_t prev_priv,
@@ -430,7 +424,7 @@ target_ulong HELPER(mret)(CPURISCVState *env)
     privilege_mode_t prev_priv = get_field(mstatus, MSTATUS_MPP);
     uintptr_t ra = GETPC();
 
-    check_ret_from_m_mode(env, retpc, prev_priv, ra);
+    check_ret_from_m_mode(env, retpc, ra);
 
     bool prev_virt = !!(get_field(env->mstatus, MSTATUS_MPV) &&
                      (prev_priv != PRV_M));
@@ -548,7 +542,7 @@ target_ulong HELPER(mnret)(CPURISCVState *env)
     bool prev_virt;
     uintptr_t ra = GETPC();
 
-    check_ret_from_m_mode(env, retpc, prev_priv, ra);
+    check_ret_from_m_mode(env, retpc, ra);
 
     prev_virt = !!(get_field(env->mnstatus, MNSTATUS_MNPV) &&
                 (prev_priv != PRV_M));
-- 
2.54.0 (Apple Git-157)

[PATCH v2 2/2] tests/tcg/riscv64: add test for mret to S-mode with no PMP entry

[tewaro]

Regression test for the previous commit. With PMP implemented and no
entry configured, the test executes mret to S-mode and checks in the
M-mode trap handler that the fault is an instruction access fault
taken from S-mode on the mret target, with mcause 1, mepc and mtval
equal to the target and MPP equal to S. It exits with 0 on success or
the number of the failed check.

Signed-off-by: tewaro <tewaro@theorem.dev>
---
v2: rebased onto current master; the meson.build entry moved to sit
between the tests added since v1. No other change.

 tests/tcg/riscv64/system/meson.build |   1 +
 tests/tcg/riscv64/test-mret-nopmp.S  | 100 +++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 tests/tcg/riscv64/test-mret-nopmp.S

diff --git a/tests/tcg/riscv64/system/meson.build b/tests/tcg/riscv64/system/meson.build
index 3c8b018d9e..f7410c6a65 100644
--- a/tests/tcg/riscv64/system/meson.build
+++ b/tests/tcg/riscv64/system/meson.build
@@ -25,6 +25,7 @@ tests += {
   'test-minstret-ecall.S': setup + {'qemu_args': ['-icount', 'shift=1', qemu_args]},
   'test-minstret-wfi.S': setup + {'qemu_args': ['-icount', 'shift=1', qemu_args]},
   'test-misa-w.S': setup + {'qemu_args': ['-cpu', 'rv64,x-misa-w=true,c=true,v=true', qemu_args]},
+  'test-mret-nopmp.S': setup + {'qemu_args': ['-cpu', 'rv64,pmp=true', qemu_args]},
   'test-zvabd.S': setup + {'qemu_args': ['-cpu', 'rv64,v=true,vlen=256,x-zvabd=true', qemu_args]},
   'test-vsepc-masking.S': setup + {
     'test_name': 'test-vsepc-masking-c',
diff --git a/tests/tcg/riscv64/test-mret-nopmp.S b/tests/tcg/riscv64/test-mret-nopmp.S
new file mode 100644
index 0000000000..df20f35bc8
--- /dev/null
+++ b/tests/tcg/riscv64/test-mret-nopmp.S
@@ -0,0 +1,100 @@
+/*
+ * Test mret to S-mode when PMP is implemented but no PMP entry is
+ * configured.
+ *
+ * The mret must succeed and the first S-mode fetch must fail [1][2].
+ * The trap is therefore taken from S-mode, with mcause 1, mepc and
+ * mtval equal to the mret target [3][4] and mstatus.MPP equal to S
+ * [5].
+ *
+ * Exit code is 0 on success, otherwise the number of the failed check.
+ *
+ * [1] Privileged ISA v1.12, 3.7.1 "Physical Memory Protection CSRs",
+ *     Priority and Matching Logic
+ * [2] Privileged ISA v1.12, 3.3.2 "Trap-Return Instructions"
+ * [3] Privileged ISA v1.12, 3.1.14 "Machine Exception Program Counter"
+ * [4] Privileged ISA v1.12, 3.1.16 "Machine Trap Value Register"
+ * [5] Privileged ISA v1.12, 3.1.6.1 "Privilege and Global
+ *     Interrupt-Enable Stack in mstatus register"
+ *
+ * Privileged ISA v1.12 (ratified, 2021-12-03):
+ * https://github.com/riscv/riscv-isa-manual/releases/tag/Priv-v1.12
+ * Current source, same wording:
+ * https://github.com/riscv/riscv-isa-manual/blob/4877e41a/src/priv/machine.adoc
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+	.option	norvc
+
+	.text
+	.global _start
+_start:
+	/* Set up machine trap vector, handle everything in M-mode */
+	lla	t0, machine_trap_handler
+	csrw	mtvec, t0
+	csrw	medeleg, zero
+
+	/* mstatus.MPP = S */
+	li	t0, 0x1800
+	csrc	mstatus, t0
+	li	t0, 0x0800
+	csrs	mstatus, t0
+
+	/* Return to S-mode at `target` with no PMP entry configured */
+	lla	t0, target
+	csrw	mepc, t0
+	mret
+
+target:
+	/* Should be unreachable with no PMP */
+	li	a0, 5
+	j	_exit
+
+machine_trap_handler:
+	/* 1: mcause is instruction access fault */
+	li	a0, 1
+	csrr	t0, mcause
+	li	t1, 1
+	bne	t0, t1, _exit
+
+	/* 2: mepc == target, not mret in _start */
+	li	a0, 2
+	csrr	t0, mepc
+	lla	t1, target
+	bne	t0, t1, _exit
+
+	/* 3: mtval == target because aligned instructions */
+	li	a0, 3
+	csrr	t0, mtval
+	bne	t0, t1, _exit
+
+	/* 4: trap from S-mode */
+	li	a0, 4
+	csrr	t0, mstatus
+	srli	t0, t0, 11
+	andi	t0, t0, 3
+	li	t1, 1
+	bne	t0, t1, _exit
+
+	li	a0, 0
+	j	_exit
+
+/* Exit with semihosting */
+_exit:
+	lla	a1, semiargs
+	li	t0, 0x20026	/* ADP_Stopped_ApplicationExit */
+	sd	t0, 0(a1)
+	sd	a0, 8(a1)
+	li	a0, 0x20	/* TARGET_SYS_EXIT_EXTENDED */
+
+	/* Semihosting call sequence */
+	slli	zero, zero, 0x1f
+	ebreak
+	srai	zero, zero, 0x7
+	j	.
+
+	.data
+	.balign	8
+semiargs:
+	.space	16
-- 
2.54.0 (Apple Git-157)