[PATCH] lib: sbi: dbtr: validate trigger configuration in update_trig

[Himanshu Chauhan]

Hi Liutong,

There are some gaps.

On Tue, Nov 03, 2026 at 08:10:01AM +0000, liutong wrote:
> sbi_dbtr_update_trig() programs a new tdata1 into an already installed
> trigger without checking it. sbi_dbtr_install_trig() runs
> dbtr_trigger_valid() on the configuration it is given, which refuses a
> tdata1 that sets the mcontrol6 M bit, because such a trigger fires
> while the hart is in M-mode. Update has no equivalent check, so S-mode
> can install a trigger that install accepts and then set that bit on it
> with an update. No race is needed: the two calls succeed in sequence
> and the bit is written to CSR_TDATA1 without being checked.
> 
> dbtr_trigger_valid() also refuses the DMODE bit. That one is not
> reachable this way, since tdata1.dmode is writable only from Debug
> Mode, but the check covers both.
> 
> Check the configuration before the trigger is reprogrammed, and copy
> it out of the shared memory once so that the validity check, the
> hardware support check and the value finally written to the CSRs all
> see the same configuration rather than separate re-reads of memory
> S-mode can still write. The copy also lets the mapping be released
> before the trigger is programmed.
> 
> SBI v3.0 section 19.5 defines SBI_ERR_INVALID_PARAM for an invalid
> trigger configuration entry. Install reports an unknown trigger type
> as SBI_ERR_FAILED instead, from a separate dbtr_trigger_supported()
> call that this patch does not add to update, so the two paths differ
> for that input.
> 
> dbtr_trigger_valid() rejects a type it does not recognise, so an
> unknown type in the new tdata1, including a zero tdata1, is refused
> here as it already was on install. It does not check the new type
> against the one the trigger was installed with, nor against the
> trigger's type_mask, so update still accepts a change of type that
> install would not have allowed in the first place.
> 
> This does not address the equivalent problem in sbi_dbtr_install_trig(),
> where the value passed to dbtr_trigger_valid() and the value later
> programmed come from separate reads of the shared memory. The
> escalation stays reachable there until that is fixed separately.
> 
> Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
> Reported-by: Nicholas Piggin <npiggin at gmail.com>
> Signed-off-by: liutong <liutong at iscas.ac.cn>
> ---
> 
> Prior art: Nicholas Piggin reported the same gap and posted a fix in
> "[PATCH 07/18] dbtr: Check for invalid and unsupported triggers in
> update" (20260313051948.4017134-8-npiggin at gmail.com). That series is
> marked changes-requested in patchwork and has had no v2 in seven
> months, and 07/18 depends on earlier patches in it, so this is a
> standalone version against current master. His version also calls
> dbtr_trigger_supported(), which is what gives install its
> SBI_ERR_FAILED for an unknown type.
> 
> Measured under QEMU 11.1.0-rc3, -M virt -cpu rv64,debug=on, one hart,
> no race, OpenSBI at c9a7d009:
> 
>   install benign (M=0)   : err=0, CSR_TDATA1 = 0x6000000000000014
>   update to M=1          : err=0, CSR_TDATA1 = 0x6000000000000054
>   after this patch       : err=-3, CSR_TDATA1 unchanged
>   benign update          : still applied
> 
> Aimed at an address OpenSBI executes, such a trigger takes a
> breakpoint in M-mode on the next ecall; an M-mode trap cannot be
> redirected, so it ends in sbi_trap_error() and sbi_hart_hang(). The
> other hart keeps running, so it is not a machine kill.
> 
> Adjacent problems this patch deliberately leaves alone, listed so they
> are not assumed fixed:
> 
> - sbi_dbtr_install_trig() has the same split between the validated
>   read and the programmed read. With a second hart rewriting the
>   entry I get the M bit into CSR_TDATA1 within a few rounds. I have a
>   tested patch that moves the check into dbtr_trigger_setup() so one
>   check covers both paths, but it restructures install_trig and I did
>   not want to put that in the same patch. Happy to send it if the
>   approach is wanted.
> - In that same loop, sbi_alloc_trigger() returning NULL leads to
>   xmit->idx = cpu_to_lle(trig->index) on a NULL trig. The comment says
>   the dry-run makes allocation infallible, but the dry-run reads the
>   entry separately.
> - sbi_dbtr_update_trig() reports no index for the failing entry, since
>   it has no out parameter, and this patch adds another failure exit to
>   it. Install sets *out on every failure. Changing the signature felt
>   out of scope here; David E. Garcia Porras's DBTR error code series
>   already proposes it.
> - update applies entries one at a time, so a refused entry leaves the
>   ones before it reprogrammed. That is pre-existing and already
>   reachable through the trig_idx check.
> 
>  lib/sbi/sbi_dbtr.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
> index 5f66b641..5cb494f0 100644
> --- a/lib/sbi/sbi_dbtr.c
> +++ b/lib/sbi/sbi_dbtr.c
> @@ -853,7 +853,8 @@ 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;
> +	unsigned long trig_idx, tdata1;
> +	struct sbi_dbtr_data_msg local;
>  	struct sbi_dbtr_trigger *trig;
>  	union sbi_dbtr_shmem_entry *entry;
>  	void *shmem_base = NULL;
> @@ -897,22 +898,34 @@ int sbi_dbtr_update_trig(unsigned long smode,
>  			return SBI_ERR_FAILED;
>  		}
>  
> -		if ((entry->data.tdata2 && !tdata2_impl) ||
> -		    (entry->data.tdata3 && !tdata3_impl)) {
> +		/*
> +		 * Copy the entry once. S-mode can rewrite the shared
> +		 * memory at any time, so the configuration that is
> +		 * checked here has to be the one that is programmed.
> +		 */
> +		local = entry->data;
> +		tdata1 = lle_to_cpu(local.tdata1);
> +
> +		if (!dbtr_trigger_valid(TDATA1_GET_TYPE(tdata1), tdata1)) {
> +			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
> +			return SBI_ERR_INVALID_PARAM;
> +		}

The SBI update rule states that trig_tdata1.type and trig_tdata1.chain must match the originally installed trigger.
The update function still allows the caller to change tdata1.type without checking whether the already allocated
hardware trigger slot supports that type. The S-mode can update an existing trigger slot into a different trigger
type/chain configuration. Install does similar check through trig->type_mask.

The updates where the new tdata1.type is not present in trig->type_mask must be rejected.

> +
> +		if ((local.tdata2 && !tdata2_impl) ||
> +		    (local.tdata3 && !tdata3_impl)) {
>  			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
>  			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))) {
> +		if (!dbtr_trigger_hw_supported(trig->index, tdata1,
> +					       lle_to_cpu(local.tdata2),
> +					       lle_to_cpu(local.tdata3))) {

dbtr_trigger_hw_supported() returns true when there is no platform callback. It is not enough.

Regards
Himanshu


>  			sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
>  			return SBI_ERR_NOT_SUPPORTED;
>  		}
>  
> -		dbtr_trigger_setup(trig, &entry->data);
>  		sbi_hart_protection_temp_unmap_range((unsigned long)entry, sizeof(*entry));
> +		dbtr_trigger_setup(trig, &local);
>  		dbtr_trigger_enable(trig);
>  	}
>  
> 
> base-commit: 2c7788e4c3efa63d670ee90402dac2a9799d0bd7
> -- 
> 2.34.1
>