[PATCH v2 2/6] lib: sbi_dbtr: fix shared memory double-fetch in install_trig

[Himanshu Chauhan]

Subject: Re: [PATCH v2 2/6] lib: sbi_dbtr: fix shared memory double-fetch in install_trig

Hi Liutong,

On Fri, Jul 31 2026, liutong wrote:

> sbi_dbtr_install_trig() reads trigger configuration from S-mode shared
> memory in two separate loops: first to validate, then to install. Since
> the shared memory remains writable by S-mode between the two reads, the
> data used for installation may differ from what was validated. This
> allows S-mode to bypass validation by modifying shared memory contents
> between the two passes, potentially installing malicious trigger
> configurations in M-mode.
>
> Fix this by merging validation and installation into a single pass.
> Each entry is copied to a local variable before use, so S-mode cannot
> modify the data between validation and installation. On validation
> failure, all previously installed triggers are rolled back.
>
> Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
> Signed-off-by: liutong <liutong at iscas.ac.cn>

Thanks for the patch series to address TOCTOU. A few things I think need to be resolved before
this can go in.

> --- a/lib/sbi/sbi_dbtr.c
> +++ b/lib/sbi/sbi_dbtr.c
> @@ -327,7 +327,7 @@ static void dbtr_trigger_setup(struct sbi_dbtr_trigger *trig,
>  	trig->tdata2 = lle_to_cpu(recv->tdata2);
>  	trig->tdata3 = lle_to_cpu(recv->tdata3);
>
> -	tdata1 = lle_to_cpu(recv->tdata1);
> +	tdata1 = trig->tdata1;

Good catch.

> @@ -603,12 +603,15 @@ int sbi_dbtr_install_trig(unsigned long smode,
>  int sbi_dbtr_install_trig(unsigned long smode,
>  			  unsigned long trig_count, unsigned long *out)
>  {
> +	struct sbi_dbtr_data_msg local;
[...]
> +	struct sbi_dbtr_trigger *installed[RV_MAX_TRIGGERS];
> +	int num_installed = 0;
> +	int ret = SBI_ERR_FAILED;
[...]
> +	if (trig_count > RV_MAX_TRIGGERS)
> +		return SBI_ERR_INVALID_PARAM;

I think it'd be worth calling out the possible stack overflow with
installed[num_installed++] explicitly in the commit. It may creep in.

> -	/* Check requested triggers configuration */
> -	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
> -		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
[...]
> +	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
> +		/*
> +		 * Snapshot one entry from shared memory so that S-mode
> +		 * cannot modify it between validation and installation.
> +		 */
> +		local = entry->data;
> +		ctrl = lle_to_cpu(local.tdata1);

This is against an older base of sbi_dbtr_install_trig() than what's
on master right now. Since the commit this fixes, we've picked up the
per-slot hardware-capability matching (dbtr_trigger_any_hw_supported())
and the dry-run slot allocation pass (dbtr_find_free_slot(), with the
`claimed` bitmask), and sbi_alloc_trigger() now takes (tdata1, tdata2,
tdata3) directly instead of being called with no args. All of those
call sites independently do lle_to_cpu(recv->tdata1/tdata2/tdata3)
straight from shared memory — they're not covered by `local` here.

So on current master this patch doesn't fully close the TOCTOU it
describes: the validate-then-install race your commit message calls
out for the check/install loops is still present between the
hw-supported check, the dry-run slot-matching loop, and the install
loop, since those all re-read `entry->data` independently. Could you
rebase onto master and thread the same `local` snapshot through
dbtr_trigger_any_hw_supported() and dbtr_find_free_slot() (e.g. pass
the already-decoded tdata1/tdata2/tdata3 instead of re-deriving them
from recv each time)? Otherwise this fixes the bug in a version of the
function that no longer exists upstream.

Also, current master distinguishes SBI_ERR_INVALID_PARAM from
SBI_ERR_FAILED for the "invalid configuration" case which this patch's
check collapses back into a single `goto rollback` with the default
`ret = SBI_ERR_FAILED`. Please make sure the rebase preserves that
distinction, e.g. by setting `ret = SBI_ERR_INVALID_PARAM` before the
goto for the dbtr_trigger_valid() failure case.

One nit pick, lower-severity and border line unexploitable though,
`local = entry->data;` copies four unsigned longs in one C statement,
but that's not an atomic access — it'll compile to separate loads.
A second hart with write access to the same physical shmem page could
in principle tear this across two different messages it wrote. It would be
worth a one-line note regarding this in the commit message

> +rollback:
> +	while (num_installed--) {
> +		dbtr_trigger_clear(installed[num_installed]);
> +		sbi_free_trigger(installed[num_installed]);
> +	}
> +	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> +					trig_count * sizeof(*entry));
> +	return ret;

Rollback ordering (reverse of install order) and the clear-before-free
sequence look correct to me.

Please rebase onto current master and extend the snapshot discipline to
the hw-supported/dry-run-allocation paths, otherwise the series doesn't
actually close the actual TOCTOU in latest code base.

Regards
Himanshu