[PATCH v3 1/3] lib: sbi: dbtr: fix shared memory double-fetch in install_trig
Anup Patel
anup at brainfault.org
Sun Oct 25 22:06:38 PDT 2026
On Mon, Oct 26, 2026 at 10:32 AM Anup Patel <anup at brainfault.org> wrote:
>
> On Thu, Sep 10, 2026 at 11:00 AM liutong <liutong at iscas.ac.cn> wrote:
> >
> > 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>
>
> LGTM.
>
> Reviewed-by: Anup Patel <anup at brainfault.org>
>
> Applied this patch to the riscv/opensbi repo.
Ignore this Reviewed-by. I have not applied this patch
due to pending review.
Regards,
Anup
>
> Thanks,
> Anup
>
> > ---
> > 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
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
More information about the opensbi
mailing list