[PATCH v3 2/3] lib: sbi: dbtr: fix shared memory double-fetch in update_trig

[liutong]

sbi_dbtr_update_trig() reads each entry from the S-mode shared memory
several times: for the trigger index, to check tdata2 and tdata3
against the implemented CSRs, for the hardware support check, and again
when the trigger is programmed. S-mode may write the shared memory
between any two of those reads, so the configuration written to the
debug CSRs is not necessarily the one that was checked.

Entries were also applied one at a time as they were validated, so a
rejected entry left the entries before it already programmed, and the
error was reported against a partially updated trigger set.

Copy the index and tdata1/tdata2/tdata3 of every entry, validate the
whole batch against that copy, and program the triggers only once all
of it has been accepted. Each entry is unmapped as soon as it has been
copied, so neither the checks nor the CSR writes can see a later value.

trig_count is already rejected when it reaches hs->total_trigs, which
cannot exceed RV_MAX_TRIGGERS, so the same check bounds the arrays.

Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
Signed-off-by: liutong <liutong at iscas.ac.cn>
---
New in v3.

sbi_dbtr_update_trig() has the same double-fetch as install_trig, and
additionally applied entries one at a time as they were validated, so a
rejected entry left the entries before it already programmed. Both are
addressed here.

 lib/sbi/sbi_dbtr.c | 63 +++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index e9135ba5..beb6d2bf 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -890,11 +890,12 @@ 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;
-	struct sbi_dbtr_trigger *trig;
+	struct sbi_dbtr_trigger *trigs[RV_MAX_TRIGGERS];
+	struct dbtr_trig_config cfg[RV_MAX_TRIGGERS];
+	struct sbi_dbtr_hart_triggers_state *hs = NULL;
 	union sbi_dbtr_shmem_entry *entry;
+	unsigned long trig_idx, i;
 	void *shmem_base = NULL;
-	struct sbi_dbtr_hart_triggers_state *hs = NULL;
 	bool tdata2_impl, tdata3_impl;
 
 	hs = dbtr_thishart_state_ptr();
@@ -906,6 +907,10 @@ int sbi_dbtr_update_trig(unsigned long smode,
 
 	shmem_base = hart_shmem_base(hs);
 
+	/*
+	 * This also bounds cfg[] and trigs[], because total_trigs can never
+	 * exceed RV_MAX_TRIGGERS.
+	 */
 	if (trig_count >= hs->total_trigs)
 		return SBI_ERR_BAD_RANGE;
 
@@ -918,41 +923,47 @@ int sbi_dbtr_update_trig(unsigned long smode,
 	tdata2_impl = tdata_implemented(CSR_TDATA2);
 	tdata3_impl = tdata_implemented(CSR_TDATA3);
 
+	/*
+	 * Snapshot and validate the whole batch before programming any of
+	 * it. Every check then acts on the same copy that is written to the
+	 * CSRs below, rather than on shared memory that S-mode can still
+	 * change, and a rejected entry cannot leave the entries before it
+	 * already applied.
+	 */
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
-		sbi_hart_protection_temp_map_range((unsigned long)entry, sizeof(*entry));
+		sbi_hart_protection_temp_map_range((unsigned long)entry,
+						   sizeof(*entry));
 		trig_idx = entry->id.idx;
+		cfg[_idx].tdata1 = lle_to_cpu(entry->data.tdata1);
+		cfg[_idx].tdata2 = lle_to_cpu(entry->data.tdata2);
+		cfg[_idx].tdata3 = lle_to_cpu(entry->data.tdata3);
+		sbi_hart_protection_temp_unmap_range((unsigned long)entry,
+						     sizeof(*entry));
 
-		if (trig_idx >= hs->total_trigs) {
-			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
+		if (trig_idx >= hs->total_trigs)
 			return SBI_ERR_INVALID_PARAM;
-		}
 
-		trig = INDEX_TO_TRIGGER(trig_idx);
+		trigs[_idx] = INDEX_TO_TRIGGER(trig_idx);
 
-		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED))) {
-			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
+		if (!(trigs[_idx]->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
 			return SBI_ERR_FAILED;
-		}
 
-		if ((entry->data.tdata2 && !tdata2_impl) ||
-		    (entry->data.tdata3 && !tdata3_impl)) {
-			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
+		if ((cfg[_idx].tdata2 && !tdata2_impl) ||
+		    (cfg[_idx].tdata3 && !tdata3_impl))
 			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))) {
-			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
+		if (!dbtr_trigger_hw_supported(trigs[_idx]->index,
+					       cfg[_idx].tdata1,
+					       cfg[_idx].tdata2,
+					       cfg[_idx].tdata3))
 			return SBI_ERR_NOT_SUPPORTED;
-		}
+	}
 
-		dbtr_trigger_setup(trig, lle_to_cpu(entry->data.tdata1),
-				   lle_to_cpu(entry->data.tdata2),
-				   lle_to_cpu(entry->data.tdata3));
-		sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
-		dbtr_trigger_enable(trig);
+	/* Program the batch only once all of it has been accepted */
+	for (i = 0; i < trig_count; i++) {
+		dbtr_trigger_setup(trigs[i], cfg[i].tdata1, cfg[i].tdata2,
+				   cfg[i].tdata3);
+		dbtr_trigger_enable(trigs[i]);
 	}
 
 	return SBI_SUCCESS;
-- 
2.34.1