[PATCH v2 0/6] Fix input validation issues in SBI ecall handlers

[PATCH v2 0/6] Fix input validation issues in SBI ecall handlers

[liutong]

This series fixes input validation and shared memory handling issues
found across multiple SBI extension handlers: DBTR, PMU, SSE, and MPXY.

The fixes fall into three categories:

1. Integer overflow in bounds checks (patches 1, 4, 6)
   Arithmetic overflow in endpoint calculations allows out-of-bounds
   access from S-mode via crafted ecall parameters.

2. TOCTOU / double-fetch from shared memory (patches 2, 5)
   S-mode shared memory is read twice --once to validate, once to apply.
   S-mode can modify the memory between reads, bypassing M-mode
   validation.

3. Insufficient address range validation (patch 3)
   Single-address domain check does not cover the full shared memory
   region, allowing cross-domain access.

These patches were previously sent individually. Per reviewer feedback,
they are now consolidated into a single series.

Changes in v2:
- Reorganized as a unified patch series (per Rahul Pathak's suggestion)
- All patches now include Fixes tags
- install_trig: fixed rollback to also cover SBI_ERR_NOT_SUPPORTED path

liutong (6):
  lib: sbi_dbtr: fix integer overflow in read_trig bounds check
  lib: sbi_dbtr: fix shared memory double-fetch in install_trig
  lib: sbi_dbtr: use range check for shared memory domain validation
  lib: sbi_pmu: fix integer overflow and zero-address in event_get_info
  lib: sbi_sse: fix shared memory double-fetch in sse_write_attrs
  lib: sbi_mpxy: fix integer overflow in attribute range endpoint

 lib/sbi/sbi_dbtr.c | 91 +++++++++++++++++++++++-----------------------
 lib/sbi/sbi_mpxy.c |  6 ++-
 lib/sbi/sbi_pmu.c  | 10 ++++-
 lib/sbi/sbi_sse.c  | 14 ++++---
 4 files changed, 66 insertions(+), 55 deletions(-)

-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 1/6] lib: sbi_dbtr: fix integer overflow in read_trig bounds check

[liutong]

In sbi_dbtr_read_trig(), the range check is:

  if (trig_idx_base + trig_count >= hs->total_trigs)

When trig_idx_base and trig_count are both unsigned long values supplied
by S-mode, their sum can wrap past ULONG_MAX to a small value, making
the check pass. For example trig_idx_base=1, trig_count=ULONG_MAX wraps
to 0, which is less than total_trigs.

This allows the subsequent for_each_trig_entry loop to access trigger
entries far beyond the triggers[] array, corrupting M-mode heap memory
via CSR read-back writes and leaking M-mode internal state to S-mode
shared memory.

Rewrite the condition as trig_count >= total_trigs - trig_idx_base. The
subtraction is safe because the preceding check already guarantees
trig_idx_base < total_trigs.

Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
Signed-off-by: liutong <liutong@iscas.ac.cn>
---

Previously sent as [PATCH].
Changes in v2:
- Added Fixes tag
- Consolidated into patch series

 lib/sbi/sbi_dbtr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 01047969..eeab7d3a 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -572,7 +572,7 @@ int sbi_dbtr_read_trig(unsigned long smode,
 		return SBI_ERR_FAILED;
 
 	if (trig_idx_base >= hs->total_trigs ||
-	    trig_idx_base + trig_count >= hs->total_trigs)
+	    trig_count >= hs->total_trigs - trig_idx_base)
 		return SBI_ERR_INVALID_PARAM;
 
 	if (sbi_dbtr_shmem_disabled(hs))
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

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

[liutong]

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.

Also add a bounds check on trig_count against RV_MAX_TRIGGERS, and fix
a redundant read in dbtr_trigger_setup() where tdata1 was read from
the message pointer a second time instead of using the value already
saved in trig->tdata1.

Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
Signed-off-by: liutong <liutong@iscas.ac.cn>
---

Previously sent as [PATCH v3].
Changes in v2:
- Single-pass with per-entry snapshot instead of full-array copy
Changes in v3:
- Added Fixes tag
Changes in v2 (series):
- Fixed rollback to cover SBI_ERR_NOT_SUPPORTED path
- Consolidated into patch series

 lib/sbi/sbi_dbtr.c | 82 ++++++++++++++++++++++------------------------
 1 file changed, 40 insertions(+), 42 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index eeab7d3a..5cca0922 100644
--- 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;
 
 	trig->state = 0;
 
@@ -603,12 +603,15 @@ int sbi_dbtr_read_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;
 	void *shmem_base = NULL;
 	union sbi_dbtr_shmem_entry *entry;
-	struct sbi_dbtr_data_msg *recv;
 	struct sbi_dbtr_id_msg *xmit;
 	unsigned long ctrl;
 	struct sbi_dbtr_trigger *trig;
+	struct sbi_dbtr_trigger *installed[RV_MAX_TRIGGERS];
+	int num_installed = 0;
+	int ret = SBI_ERR_FAILED;
 	struct sbi_dbtr_hart_triggers_state *hs = NULL;
 	bool tdata2_impl, tdata3_impl;
 
@@ -619,9 +622,13 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	if (sbi_dbtr_shmem_disabled(hs))
 		return SBI_ERR_NO_SHMEM;
 
-	shmem_base = hart_shmem_base(hs);
-	sbi_hart_protection_map_range((unsigned long)shmem_base,
-				      trig_count * sizeof(*entry));
+	if (trig_count > RV_MAX_TRIGGERS)
+		return SBI_ERR_INVALID_PARAM;
+
+	if (hs->available_trigs < trig_count) {
+		*out = hs->available_trigs;
+		return SBI_ERR_FAILED;
+	}
 
 	/*
 	 * SBI v3.0 sec 19.4 requires SBI_ERR_NOT_SUPPORTED when a trigger
@@ -632,62 +639,53 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	tdata2_impl = tdata_implemented(CSR_TDATA2);
 	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;
+	shmem_base = hart_shmem_base(hs);
+	sbi_hart_protection_map_range((unsigned long)shmem_base,
+				      trig_count * sizeof(*entry));
 
-		if (!dbtr_trigger_supported(TDATA1_GET_TYPE(ctrl))) {
-			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_FAILED;
-		}
+	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);
 
-		if (!dbtr_trigger_valid(TDATA1_GET_TYPE(ctrl), ctrl)) {
+		if (!dbtr_trigger_supported(TDATA1_GET_TYPE(ctrl)) ||
+		    !dbtr_trigger_valid(TDATA1_GET_TYPE(ctrl), ctrl)) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_FAILED;
+			goto rollback;
 		}
 
-		if ((recv->tdata2 && !tdata2_impl) ||
-		    (recv->tdata3 && !tdata3_impl)) {
+		if ((local.tdata2 && !tdata2_impl) ||
+		    (local.tdata3 && !tdata3_impl)) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_NOT_SUPPORTED;
+			ret = SBI_ERR_NOT_SUPPORTED;
+			goto rollback;
 		}
-	}
-
-	if (hs->available_trigs < trig_count) {
-		*out = hs->available_trigs;
-		sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-					       trig_count * sizeof(*entry));
-		return SBI_ERR_FAILED;
-	}
 
-	/* Install triggers */
-	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
-		/*
-		 * Since we have already checked if enough triggers are
-		 * available, trigger allocation must succeed.
-		 */
 		trig = sbi_alloc_trigger();
-
-		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
 		xmit = (struct sbi_dbtr_id_msg *)(&entry->id);
 
-		dbtr_trigger_setup(trig,  recv);
+		dbtr_trigger_setup(trig, &local);
 		dbtr_trigger_enable(trig);
 		xmit->idx = cpu_to_lle(trig->index);
-
+		installed[num_installed++] = trig;
 	}
 
 	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 					trig_count * sizeof(*entry));
 
 	return SBI_SUCCESS;
+
+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;
 }
 
 int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 3/6] lib: sbi_dbtr: use range check for shared memory domain validation

[liutong]

sbi_dbtr_setup_shmem() validates the shared memory address using
sbi_domain_check_addr(), which only checks a single address. However,
subsequent DBTR operations (install, read, update) access up to
total_trigs * sizeof(sbi_dbtr_shmem_entry) bytes starting from that
address.

If the shared memory region spans a domain boundary, accesses beyond
the first byte could violate domain isolation, allowing S-mode to
read or write M-mode memory through DBTR operations.

Replace the single-address check with sbi_domain_check_addr_range()
to validate the entire shared memory region that DBTR operations
will use.

Fixes: 23b7badeee3c ("lib: sbi: check incoming dbtr shmem address")
Signed-off-by: liutong <liutong@iscas.ac.cn>
---

Previously sent as [PATCH v2].
Changes in v2:
- Added Fixes tag
- Consolidated into patch series

 lib/sbi/sbi_dbtr.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 5cca0922..b0f1019d 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -304,9 +304,10 @@ int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
 	if (shmem_phys_hi)
 		return SBI_EINVALID_ADDR;
 
-	if (dom && !sbi_domain_check_addr(dom,
-		  DBTR_SHMEM_MAKE_PHYS(shmem_phys_hi, shmem_phys_lo), smode,
-		  SBI_DOMAIN_READ | SBI_DOMAIN_WRITE))
+	if (dom && !sbi_domain_check_addr_range(dom,
+		  DBTR_SHMEM_MAKE_PHYS(shmem_phys_hi, shmem_phys_lo),
+		  hart_state->total_trigs * sizeof(union sbi_dbtr_shmem_entry),
+		  smode, SBI_DOMAIN_READ | SBI_DOMAIN_WRITE))
 		return SBI_ERR_INVALID_ADDRESS;
 
 	hart_state->shmem.phys_lo = shmem_phys_lo;
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 4/6] lib: sbi_pmu: fix integer overflow and zero-address in event_get_info

[liutong]

sbi_pmu_event_get_info() computes the shared memory size as
num_events * sizeof(struct sbi_pmu_event_info) without checking for
integer overflow. A sufficiently large num_events causes the product to
wrap around, making the domain check pass on a truncated size while the
loop iterates with the original count.

This allows S-mode to trigger out-of-bounds writes in M-mode memory.
A zero physical address is also not rejected, which would cause M-mode
to write to address 0 and hang the hart.

Add bounds checking on num_events before the multiplication and reject
zero shmem addresses early.

Fixes: e4345842168b ("lib: sbi_pmu: Implement SBI PMU event info function")
Signed-off-by: liutong <liutong@iscas.ac.cn>
---

Previously sent as [PATCH v2].
Changes in v2:
- Added Fixes tag
- Consolidated into patch series

 lib/sbi/sbi_pmu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index a0f6d2fa..3a2321cc 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -1061,7 +1061,7 @@ int sbi_pmu_ctr_get_info(uint32_t cidx, unsigned long *ctr_info)
 int sbi_pmu_event_get_info(unsigned long shmem_phys_lo, unsigned long shmem_phys_hi,
 			   unsigned long num_events, unsigned long flags)
 {
-	unsigned long shmem_size = num_events * sizeof(struct sbi_pmu_event_info);
+	unsigned long shmem_size;
 	int i, j, event_type;
 	struct sbi_pmu_event_info *einfo;
 	struct sbi_pmu_hart_state *phs = pmu_thishart_state_ptr();
@@ -1076,6 +1076,14 @@ int sbi_pmu_event_get_info(unsigned long shmem_phys_lo, unsigned long shmem_phys
 	if (!num_events || (shmem_phys_lo & 0xF))
 		return SBI_ERR_INVALID_PARAM;
 
+	if (!shmem_phys_lo)
+		return SBI_ERR_INVALID_ADDRESS;
+
+	if (num_events > ((unsigned long)-1) / sizeof(struct sbi_pmu_event_info))
+		return SBI_ERR_INVALID_PARAM;
+
+	shmem_size = num_events * sizeof(struct sbi_pmu_event_info);
+
 	/*
 	 * On RV32, the M-mode can only access the first 4GB of
 	 * the physical address space because M-mode does not have
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 5/6] lib: sbi_sse: fix shared memory double-fetch in sse_write_attrs

[liutong]

sse_write_attrs() reads attribute values from S-mode shared memory in
two passes: first to validate, then to apply. Since the shared memory
remains writable by S-mode between the two reads, the values used for
application may differ from what was validated.

This allows S-mode to bypass validation by modifying shared memory
contents between the two passes, potentially setting unauthorized SSE
event attributes in M-mode.

Fix this by snapshotting the shared memory data into a local buffer
and performing both validation and application against that snapshot.

Fixes: c8cdf01d8f3a ("lib: sbi: Add support for Supervisor Software Events extension")
Signed-off-by: liutong <liutong@iscas.ac.cn>
---

Previously sent as [PATCH v2].
Changes in v2:
- Added Fixes tag
- Consolidated into patch series

 lib/sbi/sbi_sse.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/sbi/sbi_sse.c b/lib/sbi/sbi_sse.c
index 818afb87..94b333c5 100644
--- a/lib/sbi/sbi_sse.c
+++ b/lib/sbi/sbi_sse.c
@@ -1064,25 +1064,27 @@ static int sse_write_attrs(struct sbi_sse_event *e, uint32_t base_attr_id,
 	unsigned long attr = 0, val;
 	uint32_t id, end_id = base_attr_id + attr_count;
 	unsigned long *attrs = (unsigned long *)input_phys;
+	unsigned long local_attrs[SBI_SSE_ATTR_MAX];
 
 	sbi_hart_protection_map_range(input_phys, sizeof(unsigned long) * attr_count);
 
+	copy_attrs(local_attrs, attrs, attr_count);
+
+	sbi_hart_protection_unmap_range(input_phys, sizeof(unsigned long) * attr_count);
+
 	for (id = base_attr_id; id < end_id; id++) {
-		val = attrs[attr++];
+		val = local_attrs[attr++];
 		ret = sse_event_set_attr_check(e, id, val);
 		if (ret)
-			goto out;
+			return ret;
 	}
 
 	attr = 0;
 	for (id = base_attr_id; id < end_id; id++) {
-		val = attrs[attr++];
+		val = local_attrs[attr++];
 		sse_event_set_attr(e, id, val);
 	}
 
-out:
-	sbi_hart_protection_unmap_range(input_phys, sizeof(unsigned long) * attr_count);
-
 	return ret;
 }
 
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 6/6] lib: sbi_mpxy: fix integer overflow in attribute range endpoint

[liutong]

In sbi_mpxy_read_attrs() and sbi_mpxy_write_attrs(), the attribute range
endpoint is computed as:

  u32 end_id = base_attr_id + attr_count - 1;

When base_attr_id + attr_count exceeds UINT32_MAX, end_id wraps around to
a small value. This makes downstream range checks such as
"end_id >= SBI_MPXY_ATTR_STD_ATTR_MAX_IDX" pass incorrectly, turning them
into dead code.

A malicious or misconfigured S-mode caller can supply crafted
base_attr_id and attr_count values that trigger the wraparound, bypassing
range validation and causing out-of-bounds read/write on the shared
memory attribute array.

Widen end_id to u64 so the addition cannot wrap within the u32 range.

Fixes: 7939bf1329eb ("lib: sbi: Add SBI Message Proxy (MPXY) framework")
Signed-off-by: liutong <liutong@iscas.ac.cn>
---

Previously sent as [PATCH v2].
Changes in v2:
- Added Fixes tag
- Consolidated into patch series

 lib/sbi/sbi_mpxy.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/sbi/sbi_mpxy.c b/lib/sbi/sbi_mpxy.c
index 19f59f3a..37dc8e7e 100644
--- a/lib/sbi/sbi_mpxy.c
+++ b/lib/sbi/sbi_mpxy.c
@@ -475,7 +475,8 @@ int sbi_mpxy_read_attrs(u32 channel_id, u32 base_attr_id, u32 attr_count)
 {
 	struct mpxy_state *ms = sbi_domain_mpxy_state_thishart_ptr();
 	int ret = SBI_SUCCESS;
-	u32 *attr_ptr, end_id;
+	u32 *attr_ptr;
+	u64 end_id;
 	void *shmem_base;
 	struct sbi_domain *dom = sbi_domain_thishart_ptr();
 
@@ -625,7 +626,8 @@ static void mpxy_write_std_attr(struct sbi_mpxy_channel *channel, u32 attr_id,
 int sbi_mpxy_write_attrs(u32 channel_id, u32 base_attr_id, u32 attr_count)
 {
 	struct mpxy_state *ms = sbi_domain_mpxy_state_thishart_ptr();
-	u32 *mem_ptr, attr_id, end_id, attr_val;
+	u32 *mem_ptr, attr_id, attr_val;
+	u64 end_id;
 	struct sbi_mpxy_channel *channel;
 	int ret, mem_idx;
 	void *shmem_base;
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH v2 1/6] lib: sbi_dbtr: fix integer overflow in read_trig bounds check

[Himanshu Chauhan]

On Fri, Jul 31, 2026 at 10:34:00AM +0000, liutong wrote:
> In sbi_dbtr_read_trig(), the range check is:
> 
>   if (trig_idx_base + trig_count >= hs->total_trigs)
> 
> When trig_idx_base and trig_count are both unsigned long values supplied
> by S-mode, their sum can wrap past ULONG_MAX to a small value, making
> the check pass. For example trig_idx_base=1, trig_count=ULONG_MAX wraps
> to 0, which is less than total_trigs.
> 
> This allows the subsequent for_each_trig_entry loop to access trigger
> entries far beyond the triggers[] array, corrupting M-mode heap memory
> via CSR read-back writes and leaking M-mode internal state to S-mode
> shared memory.
> 
> Rewrite the condition as trig_count >= total_trigs - trig_idx_base. The
> subtraction is safe because the preceding check already guarantees
> trig_idx_base < total_trigs.
> 
> Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
> Signed-off-by: liutong <liutong@iscas.ac.cn>
> ---
> 
> Previously sent as [PATCH].
> Changes in v2:
> - Added Fixes tag
> - Consolidated into patch series
> 
>  lib/sbi/sbi_dbtr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
> index 01047969..eeab7d3a 100644
> --- a/lib/sbi/sbi_dbtr.c
> +++ b/lib/sbi/sbi_dbtr.c
> @@ -572,7 +572,7 @@ int sbi_dbtr_read_trig(unsigned long smode,
>  		return SBI_ERR_FAILED;
>  
>  	if (trig_idx_base >= hs->total_trigs ||
> -	    trig_idx_base + trig_count >= hs->total_trigs)
> +	    trig_count >= hs->total_trigs - trig_idx_base)
>  		return SBI_ERR_INVALID_PARAM;
Looks good to me.

Reviewed-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>

>  
>  	if (sbi_dbtr_shmem_disabled(hs))
> -- 
> 2.34.1
> 
> 
> -- 
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [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@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

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH v2 3/6] lib: sbi_dbtr: use range check for shared memory domain validation

[Himanshu Chauhan]

On Fri, Jul 31, 2026 at 10:34:02AM +0000, liutong wrote:
> sbi_dbtr_setup_shmem() validates the shared memory address using
> sbi_domain_check_addr(), which only checks a single address. However,
> subsequent DBTR operations (install, read, update) access up to
> total_trigs * sizeof(sbi_dbtr_shmem_entry) bytes starting from that
> address.
> 
> If the shared memory region spans a domain boundary, accesses beyond
> the first byte could violate domain isolation, allowing S-mode to
> read or write M-mode memory through DBTR operations.
> 
> Replace the single-address check with sbi_domain_check_addr_range()
> to validate the entire shared memory region that DBTR operations
> will use.
> 
> Fixes: 23b7badeee3c ("lib: sbi: check incoming dbtr shmem address")
> Signed-off-by: liutong <liutong@iscas.ac.cn>
> ---
> 
> Previously sent as [PATCH v2].
> Changes in v2:
> - Added Fixes tag
> - Consolidated into patch series
> 
>  lib/sbi/sbi_dbtr.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
> index 5cca0922..b0f1019d 100644
> --- a/lib/sbi/sbi_dbtr.c
> +++ b/lib/sbi/sbi_dbtr.c
> @@ -304,9 +304,10 @@ int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
>  	if (shmem_phys_hi)
>  		return SBI_EINVALID_ADDR;
>  
> -	if (dom && !sbi_domain_check_addr(dom,
> -		  DBTR_SHMEM_MAKE_PHYS(shmem_phys_hi, shmem_phys_lo), smode,
> -		  SBI_DOMAIN_READ | SBI_DOMAIN_WRITE))
> +	if (dom && !sbi_domain_check_addr_range(dom,
> +		  DBTR_SHMEM_MAKE_PHYS(shmem_phys_hi, shmem_phys_lo),
> +		  hart_state->total_trigs * sizeof(union sbi_dbtr_shmem_entry),
> +		  smode, SBI_DOMAIN_READ | SBI_DOMAIN_WRITE))
>  		return SBI_ERR_INVALID_ADDRESS;
>
one nit pick: The dom && guard means the range check is skipped entirely if dom is NULL. Since you are
trying to close gaps, I would suggest that you add a check for dom.

Regards
Himanshu
  
>  	hart_state->shmem.phys_lo = shmem_phys_lo;
> -- 
> 2.34.1
> 
> 
> -- 
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH v2 5/6] lib: sbi_sse: fix shared memory double-fetch in sse_write_attrs

[Himanshu Chauhan]

On Fri, Jul 31, 2026 at 10:34:04AM +0000, liutong wrote:
> sse_write_attrs() reads attribute values from S-mode shared memory in
> two passes: first to validate, then to apply. Since the shared memory
> remains writable by S-mode between the two reads, the values used for
> application may differ from what was validated. This allows S-mode to
> bypass validation by modifying shared memory contents between the two
> passes, potentially setting unauthorized SSE event attributes in
> M-mode.
>
> Fix this by snapshotting the shared memory data into a local buffer
> and performing both validation and application against that snapshot.
>
> Fixes: c8cdf01d8f3a ("lib: sbi: Add support for Supervisor Software Events extension")
> Signed-off-by: liutong <liutong@iscas.ac.cn>

> @@ -1064,25 +1064,27 @@ static int sse_write_attrs(struct sbi_sse_event *e, uint32_t base_attr_id,
>  	unsigned long attr = 0, val;
>  	uint32_t id, end_id = base_attr_id + attr_count;
>  	unsigned long *attrs = (unsigned long *)input_phys;
> +	unsigned long local_attrs[SBI_SSE_ATTR_MAX];
>
>  	sbi_hart_protection_map_range(input_phys, sizeof(unsigned long) * attr_count);
>
> +	copy_attrs(local_attrs, attrs, attr_count);
> +
> +	sbi_hart_protection_unmap_range(input_phys, sizeof(unsigned long) * attr_count);
> +
>  	for (id = base_attr_id; id < end_id; id++) {
> -		val = attrs[attr++];
> +		val = local_attrs[attr++];
>  		ret = sse_event_set_attr_check(e, id, val);
>  		if (ret)
> -			goto out;
> +			return ret;
>  	}

One thing I do want resolved before this goes in: attr_count is only
bounded by SBI_SSE_ATTR_MAX (10) via sbi_sse_attr_check() in the
caller, sbi_sse_write_attrs() — sse_write_attrs() itself has no
internal check. That's fine today because it's a static function with
exactly one call site, always reached after that check passes. But
this patch changes what an unbounded attr_count would do here: before,
attrs[attr++] would walk off the end of the *shared-memory* mapping;
after, copy_attrs() writes attr_count longs into a fixed 10-entry
*stack* array. If that caller-side invariant is ever violated by a
future call site or reordering, this is now a stack buffer overflow
instead of an OOB read of mapped memory — a materially worse failure
mode, introduced by adding local_attrs.

Given this whole series is about not trusting validation that happened
somewhere else against data that's used here, I think sse_write_attrs()
should defend itself directly rather than relying entirely on the
caller:

	if (attr_count > SBI_SSE_ATTR_MAX)
		return SBI_ERR_INVALID_PARAM;

right before the copy_attrs() call (or equivalently, clamp/assert
before sizing the copy). Could you add that in the next version?

> +	sbi_hart_protection_unmap_range(input_phys, sizeof(unsigned long) * attr_count);
> +
>  	return ret;
>  }

Rest looks good.

Regards
Himanshu

>  
> -- 
> 2.34.1
> 
> 
> -- 
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi