[PATCH v3 1/3] lib: sbi: dbtr: fix shared memory double-fetch in install_trig
Himanshu Chauhan
himanshu.chauhan at oss.qualcomm.com
Sun Sep 20 21:56:10 PDT 2026
Hi Liutong,
On Thu, Sep 10, 2026 at 05:29:56AM +0000, liutong 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>
> ---
> 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];
I am skeptical to take big structures on stack.
> + 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);
> + }
> +
The copy and the basic sanity check can be combined here.
> /*
> * 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);
>
This check can be moved up in the previous loop and fail early.
> - 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;
> }
ditto.
>
> - 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;
ditto.
> }
>
> - 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;
> + }
Since the dry-run succeeded, the sbi_alloc_trigger will not fail here.
>
> - 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);
Not needed as said above.
> }
>
> + 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);
Rollback doesn't make sense because of dry-run.
The basic premise that the read/write between S/M mode is not atomic and other CPUs can
still overwrite other CPUs' state is still not addressed. The current CPU can't do anything.
So looks like a S-mode problem we are trying to solve in m-mode without much gain.
Looking at the sheer amount of code churn that this patch does is too much.
A new patch series which merges the sanity and just the allocation of triggers in one go and installation
on another would make sense.
I would NAK this patch.
Regards
Himanshu
> + 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