[PATCH] lib: sbi: dbtr: validate trigger configuration in update_trig

[liutong]

sbi_dbtr_update_trig() programs a new tdata1 into an already installed
trigger without checking it. sbi_dbtr_install_trig() runs
dbtr_trigger_valid() on the configuration it is given, which refuses a
tdata1 that sets the mcontrol6 M bit, because such a trigger fires
while the hart is in M-mode. Update has no equivalent check, so S-mode
can install a trigger that install accepts and then set that bit on it
with an update. No race is needed: the two calls succeed in sequence
and the bit is written to CSR_TDATA1 without being checked.

dbtr_trigger_valid() also refuses the DMODE bit. That one is not
reachable this way, since tdata1.dmode is writable only from Debug
Mode, but the check covers both.

Check the configuration before the trigger is reprogrammed, and copy
it out of the shared memory once so that the validity check, the
hardware support check and the value finally written to the CSRs all
see the same configuration rather than separate re-reads of memory
S-mode can still write. The copy also lets the mapping be released
before the trigger is programmed.

SBI v3.0 section 19.5 defines SBI_ERR_INVALID_PARAM for an invalid
trigger configuration entry. Install reports an unknown trigger type
as SBI_ERR_FAILED instead, from a separate dbtr_trigger_supported()
call that this patch does not add to update, so the two paths differ
for that input.

dbtr_trigger_valid() rejects a type it does not recognise, so an
unknown type in the new tdata1, including a zero tdata1, is refused
here as it already was on install. It does not check the new type
against the one the trigger was installed with, nor against the
trigger's type_mask, so update still accepts a change of type that
install would not have allowed in the first place.

This does not address the equivalent problem in sbi_dbtr_install_trig(),
where the value passed to dbtr_trigger_valid() and the value later
programmed come from separate reads of the shared memory. The
escalation stays reachable there until that is fixed separately.

Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
Reported-by: Nicholas Piggin <npiggin at gmail.com>
Signed-off-by: liutong <liutong at iscas.ac.cn>
---

Prior art: Nicholas Piggin reported the same gap and posted a fix in
"[PATCH 07/18] dbtr: Check for invalid and unsupported triggers in
update" (20260313051948.4017134-8-npiggin at gmail.com). That series is
marked changes-requested in patchwork and has had no v2 in seven
months, and 07/18 depends on earlier patches in it, so this is a
standalone version against current master. His version also calls
dbtr_trigger_supported(), which is what gives install its
SBI_ERR_FAILED for an unknown type.

Measured under QEMU 11.1.0-rc3, -M virt -cpu rv64,debug=on, one hart,
no race, OpenSBI at c9a7d009:

  install benign (M=0)   : err=0, CSR_TDATA1 = 0x6000000000000014
  update to M=1          : err=0, CSR_TDATA1 = 0x6000000000000054
  after this patch       : err=-3, CSR_TDATA1 unchanged
  benign update          : still applied

Aimed at an address OpenSBI executes, such a trigger takes a
breakpoint in M-mode on the next ecall; an M-mode trap cannot be
redirected, so it ends in sbi_trap_error() and sbi_hart_hang(). The
other hart keeps running, so it is not a machine kill.

Adjacent problems this patch deliberately leaves alone, listed so they
are not assumed fixed:

- sbi_dbtr_install_trig() has the same split between the validated
  read and the programmed read. With a second hart rewriting the
  entry I get the M bit into CSR_TDATA1 within a few rounds. I have a
  tested patch that moves the check into dbtr_trigger_setup() so one
  check covers both paths, but it restructures install_trig and I did
  not want to put that in the same patch. Happy to send it if the
  approach is wanted.
- In that same loop, sbi_alloc_trigger() returning NULL leads to
  xmit->idx = cpu_to_lle(trig->index) on a NULL trig. The comment says
  the dry-run makes allocation infallible, but the dry-run reads the
  entry separately.
- sbi_dbtr_update_trig() reports no index for the failing entry, since
  it has no out parameter, and this patch adds another failure exit to
  it. Install sets *out on every failure. Changing the signature felt
  out of scope here; David E. Garcia Porras's DBTR error code series
  already proposes it.
- update applies entries one at a time, so a refused entry leaves the
  ones before it reprogrammed. That is pre-existing and already
  reachable through the trig_idx check.

 lib/sbi/sbi_dbtr.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 5f66b641..5cb494f0 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -853,7 +853,8 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
 int sbi_dbtr_update_trig(unsigned long smode,
 			 unsigned long trig_count)
 {
-	unsigned long trig_idx;
+	unsigned long trig_idx, tdata1;
+	struct sbi_dbtr_data_msg local;
 	struct sbi_dbtr_trigger *trig;
 	union sbi_dbtr_shmem_entry *entry;
 	void *shmem_base = NULL;
@@ -897,22 +898,34 @@ int sbi_dbtr_update_trig(unsigned long smode,
 			return SBI_ERR_FAILED;
 		}
 
-		if ((entry->data.tdata2 && !tdata2_impl) ||
-		    (entry->data.tdata3 && !tdata3_impl)) {
+		/*
+		 * Copy the entry once. S-mode can rewrite the shared
+		 * memory at any time, so the configuration that is
+		 * checked here has to be the one that is programmed.
+		 */
+		local = entry->data;
+		tdata1 = lle_to_cpu(local.tdata1);
+
+		if (!dbtr_trigger_valid(TDATA1_GET_TYPE(tdata1), tdata1)) {
+			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
+			return SBI_ERR_INVALID_PARAM;
+		}
+
+		if ((local.tdata2 && !tdata2_impl) ||
+		    (local.tdata3 && !tdata3_impl)) {
 			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
 			return SBI_ERR_NOT_SUPPORTED;
 		}
 
-		if (!dbtr_trigger_hw_supported(trig->index,
-					       lle_to_cpu(entry->data.tdata1),
-					       lle_to_cpu(entry->data.tdata2),
-					       lle_to_cpu(entry->data.tdata3))) {
+		if (!dbtr_trigger_hw_supported(trig->index, tdata1,
+					       lle_to_cpu(local.tdata2),
+					       lle_to_cpu(local.tdata3))) {
 			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
 			return SBI_ERR_NOT_SUPPORTED;
 		}
 
-		dbtr_trigger_setup(trig, &entry->data);
 		sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
+		dbtr_trigger_setup(trig, &local);
 		dbtr_trigger_enable(trig);
 	}
 

base-commit: 2c7788e4c3efa63d670ee90402dac2a9799d0bd7
-- 
2.34.1