[PATCH v3 1/3] lib: sbi: dbtr: fix shared memory double-fetch in install_trig
liutong
liutong at iscas.ac.cn
Wed Sep 9 22:29:56 PDT 2026
sbi_dbtr_install_trig() reads each trigger configuration from the
S-mode shared memory in three passes: the configuration check, the
dry-run slot allocation, and the install loop. Each pass re-reads
entry->data, which S-mode may write in between, so the configuration
that gets programmed is not necessarily the one that was checked.
dbtr_trigger_valid() does reject the M-mode enable bit, but it can be
checking a different value than the one installed.
Copy tdata1/tdata2/tdata3 for the whole request into a local array once
and run all three passes against that copy. The shared memory is then
only read while taking the copy, and only written to report the
allocated trigger indices. dbtr_trigger_setup() takes the decoded
values directly, which also covers what dbtr_find_free_slot() and
sbi_alloc_trigger() are given.
cfg[] is a fixed size array, so trig_count is bounded before anything
is copied into it. total_trigs cannot exceed RV_MAX_TRIGGERS, so
rejecting trig_count against total_trigs bounds cfg[] too. Rollback
records the installed slots in a u32 bitmap rather than an array, so it
needs no bound of its own.
The loads that fill one entry are not atomic, so a second hart writing
the same page can still tear an entry. That does not reintroduce the
problem here, since all passes then agree on one copy.
Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
Signed-off-by: liutong <liutong at iscas.ac.cn>
---
Previously sent as [PATCH v2 2/6].
Changes in v3:
- Rebased onto current master. The v2 version predated the per-slot
capability matching and the dry-run slot allocation, so it no longer
matched the function it was fixing.
- The copy is threaded through dbtr_trigger_any_hw_supported(),
dbtr_find_free_slot() and sbi_alloc_trigger(). dbtr_trigger_setup()
now takes the decoded values instead of re-reading the message.
- Preserved master's SBI_ERR_INVALID_PARAM vs SBI_ERR_FAILED
distinction for the invalid configuration case.
- Rollback state is a u32 slot bitmap rather than an array of installed
triggers, so the out-of-bounds write that was a concern with
installed[num_installed++] no longer exists.
- The bound is now "trig_count >= hs->total_trigs" returning
SBI_ERR_BAD_RANGE, the same shape as the check in Pengpeng Hou's
"lib: sbi: dbtr: validate complete shared memory range" and in the
DBTR error code rework, rather than a separate RV_MAX_TRIGGERS test
that would have collided with both.
- Noted in the commit message that the loads are not atomic, and what
that does and does not mean for this fix.
lib/sbi/sbi_dbtr.c | 169 ++++++++++++++++++++++++++++-----------------
1 file changed, 104 insertions(+), 65 deletions(-)
diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 7e4ddd98..e9135ba5 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -392,18 +392,15 @@ int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
}
static void dbtr_trigger_setup(struct sbi_dbtr_trigger *trig,
- struct sbi_dbtr_data_msg *recv)
+ unsigned long tdata1, unsigned long tdata2,
+ unsigned long tdata3)
{
- unsigned long tdata1;
-
if (!trig)
return;
- trig->tdata1 = lle_to_cpu(recv->tdata1);
- trig->tdata2 = lle_to_cpu(recv->tdata2);
- trig->tdata3 = lle_to_cpu(recv->tdata3);
-
- tdata1 = lle_to_cpu(recv->tdata1);
+ trig->tdata1 = tdata1;
+ trig->tdata2 = tdata2;
+ trig->tdata3 = tdata3;
trig->state = 0;
@@ -677,19 +674,30 @@ int sbi_dbtr_read_trig(unsigned long smode,
return SBI_SUCCESS;
}
+/*
+ * Trigger configuration decoded from the shared memory. S-mode can write
+ * the shared memory at any time, so a request is snapshotted once and all
+ * later passes work on the snapshot instead of the shared memory.
+ */
+struct dbtr_trig_config {
+ unsigned long tdata1;
+ unsigned long tdata2;
+ unsigned long tdata3;
+};
+
int sbi_dbtr_install_trig(unsigned long smode,
unsigned long trig_count, unsigned long *out)
{
- void *shmem_base = NULL;
+ struct dbtr_trig_config cfg[RV_MAX_TRIGGERS];
+ struct sbi_dbtr_hart_triggers_state *hs = NULL;
union sbi_dbtr_shmem_entry *entry;
- struct sbi_dbtr_data_msg *recv;
- struct sbi_dbtr_id_msg *xmit;
- unsigned long ctrl;
- u32 claimed = 0;
- int slot;
struct sbi_dbtr_trigger *trig;
- struct sbi_dbtr_hart_triggers_state *hs = NULL;
+ struct sbi_dbtr_id_msg *xmit;
bool tdata2_impl, tdata3_impl;
+ void *shmem_base = NULL;
+ u32 claimed = 0, installed = 0;
+ unsigned long i;
+ int slot, ret;
hs = dbtr_thishart_state_ptr();
if (!hs)
@@ -698,10 +706,29 @@ int sbi_dbtr_install_trig(unsigned long smode,
if (sbi_dbtr_shmem_disabled(hs))
return SBI_ERR_NO_SHMEM;
+ /*
+ * This also bounds cfg[], because total_trigs can never exceed
+ * RV_MAX_TRIGGERS.
+ */
+ if (trig_count >= hs->total_trigs)
+ return SBI_ERR_BAD_RANGE;
+
shmem_base = hart_shmem_base(hs);
sbi_hart_protection_temp_map_range((unsigned long)shmem_base,
trig_count * sizeof(*entry));
+ /*
+ * Snapshot the whole request before looking at it. These loads are
+ * not atomic, so a second hart writing the same page can still tear
+ * an entry across two messages it wrote, but it can no longer make
+ * M-mode install a configuration that was never validated.
+ */
+ for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
+ 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 v3.0 sec 19.4 requires SBI_ERR_NOT_SUPPORTED when a trigger
* programs a non-zero value into an unimplemented optional CSR. Only
@@ -714,48 +741,41 @@ int sbi_dbtr_install_trig(unsigned long smode,
tdata3_impl = tdata_implemented(CSR_TDATA3);
/* Check requested triggers configuration */
- for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
- recv = (struct sbi_dbtr_data_msg *)(&entry->data);
- ctrl = recv->tdata1;
+ for (i = 0; i < trig_count; i++) {
+ unsigned long type = TDATA1_GET_TYPE(cfg[i].tdata1);
- if (!dbtr_trigger_supported(TDATA1_GET_TYPE(ctrl))) {
- *out = _idx;
- sbi_hart_protection_temp_unmap_range((unsigned long)shmem_base,
- trig_count * sizeof(*entry));
- return SBI_ERR_FAILED;
+ if (!dbtr_trigger_supported(type)) {
+ *out = i;
+ ret = SBI_ERR_FAILED;
+ goto out;
}
- if (!dbtr_trigger_valid(TDATA1_GET_TYPE(ctrl), ctrl)) {
- *out = _idx;
- sbi_hart_protection_temp_unmap_range((unsigned long)shmem_base,
- trig_count * sizeof(*entry));
- return SBI_ERR_INVALID_PARAM;
+ if (!dbtr_trigger_valid(type, cfg[i].tdata1)) {
+ *out = i;
+ ret = SBI_ERR_INVALID_PARAM;
+ goto out;
}
- if ((recv->tdata2 && !tdata2_impl) ||
- (recv->tdata3 && !tdata3_impl)) {
- *out = _idx;
- sbi_hart_protection_temp_unmap_range((unsigned long)shmem_base,
- trig_count * sizeof(*entry));
- return SBI_ERR_NOT_SUPPORTED;
+ if ((cfg[i].tdata2 && !tdata2_impl) ||
+ (cfg[i].tdata3 && !tdata3_impl)) {
+ *out = i;
+ ret = SBI_ERR_NOT_SUPPORTED;
+ goto out;
}
- if (!dbtr_trigger_any_hw_supported(hs,
- lle_to_cpu(recv->tdata1),
- lle_to_cpu(recv->tdata2),
- lle_to_cpu(recv->tdata3))) {
- *out = _idx;
- sbi_hart_protection_temp_unmap_range((unsigned long)shmem_base,
- trig_count * sizeof(*entry));
- return SBI_ERR_NOT_SUPPORTED;
+ if (!dbtr_trigger_any_hw_supported(hs, cfg[i].tdata1,
+ cfg[i].tdata2,
+ cfg[i].tdata3)) {
+ *out = i;
+ ret = SBI_ERR_NOT_SUPPORTED;
+ goto out;
}
}
if (hs->available_trigs < trig_count) {
*out = hs->available_trigs;
- sbi_hart_protection_temp_unmap_range((unsigned long)shmem_base,
- trig_count * sizeof(*entry));
- return SBI_ERR_FAILED;
+ ret = SBI_ERR_FAILED;
+ goto out;
}
/*
@@ -763,45 +783,62 @@ int sbi_dbtr_install_trig(unsigned long smode,
* is installed if any of the requested configurations cannot be
* matched to a free hardware trigger slot.
*/
- for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
- recv = (struct sbi_dbtr_data_msg *)(&entry->data);
- slot = dbtr_find_free_slot(hs, claimed,
- lle_to_cpu(recv->tdata1),
- lle_to_cpu(recv->tdata2),
- lle_to_cpu(recv->tdata3));
+ for (i = 0; i < trig_count; i++) {
+ slot = dbtr_find_free_slot(hs, claimed, cfg[i].tdata1,
+ cfg[i].tdata2, cfg[i].tdata3);
if (slot < 0) {
- *out = _idx;
- sbi_hart_protection_temp_unmap_range((unsigned long)shmem_base,
- trig_count * sizeof(*entry));
- return SBI_ERR_FAILED;
+ *out = i;
+ ret = SBI_ERR_FAILED;
+ goto out;
}
claimed |= BIT(slot);
}
/* Install triggers */
for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
- recv = (struct sbi_dbtr_data_msg *)(&entry->data);
xmit = (struct sbi_dbtr_id_msg *)(&entry->id);
/*
* The dry-run above matched every requested configuration
- * to a free hardware trigger slot, so allocation must
- * succeed.
+ * to a free hardware trigger slot, so this cannot fail.
+ * Unwind instead of relying on that invariant, so that a
+ * later change to the matching logic cannot leave a
+ * half-installed batch behind or reach the NULL
+ * dereference in the index write-back below.
*/
- trig = sbi_alloc_trigger(lle_to_cpu(recv->tdata1),
- lle_to_cpu(recv->tdata2),
- lle_to_cpu(recv->tdata3));
+ trig = sbi_alloc_trigger(cfg[_idx].tdata1, cfg[_idx].tdata2,
+ cfg[_idx].tdata3);
+ if (!trig) {
+ *out = _idx;
+ ret = SBI_ERR_FAILED;
+ goto rollback;
+ }
- dbtr_trigger_setup(trig, recv);
+ dbtr_trigger_setup(trig, cfg[_idx].tdata1, cfg[_idx].tdata2,
+ cfg[_idx].tdata3);
dbtr_trigger_enable(trig);
xmit->idx = cpu_to_lle(trig->index);
-
+ installed |= BIT(trig->index);
}
+ ret = SBI_SUCCESS;
+ goto out;
+
+rollback:
+ /* Undo the batch in reverse order of installation */
+ for (i = RV_MAX_TRIGGERS; i-- > 0;) {
+ if (!(installed & BIT(i)))
+ continue;
+
+ trig = INDEX_TO_TRIGGER(i);
+ dbtr_trigger_clear(trig);
+ sbi_free_trigger(trig);
+ }
+out:
sbi_hart_protection_temp_unmap_range((unsigned long)shmem_base,
trig_count * sizeof(*entry));
- return SBI_SUCCESS;
+ return ret;
}
int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
@@ -911,7 +948,9 @@ int sbi_dbtr_update_trig(unsigned long smode,
return SBI_ERR_NOT_SUPPORTED;
}
- dbtr_trigger_setup(trig, &entry->data);
+ 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);
}
--
2.34.1
More information about the opensbi
mailing list