[PATCH v3 3/3] lib: sbi: sse: fix shared memory double-fetch in sse_write_attrs

[liutong]

sse_write_attrs() reads the attribute values from the S-mode shared
memory in two passes: first to validate them, then to apply them.
S-mode may write the shared memory between the passes, so a value that
sse_event_set_attr_check() would have rejected can still be applied.

Copy the attributes into a local array, unmap the shared memory right
away, and run both passes against that copy.

The copy is a fixed size stack array. Its bound is currently guaranteed
by sbi_sse_attr_check() in the only caller, but relying on that would
turn a future unchecked call site into a stack buffer overflow, where
today it would only read past the shared mapping. Check attr_count here
as well, so the function does not depend on validation done elsewhere.

Fixes: c8cdf01d8f3a ("lib: sbi: Add support for Supervisor Software Events extension")
Suggested-by: Himanshu Chauhan <himanshu.chauhan at oss.qualcomm.com>
Signed-off-by: liutong <liutong at iscas.ac.cn>
---
Previously sent as [PATCH v2 5/6].

Changes in v3:
- Rebased onto current master (sbi_hart_protection_*_range rename)
- Added the attr_count > SBI_SSE_ATTR_MAX check inside
  sse_write_attrs(), so that it does not depend on the bound being
  established by its caller
- Subject prefix changed to "lib: sbi: sse:"

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

diff --git a/lib/sbi/sbi_sse.c b/lib/sbi/sbi_sse.c
index f6d0111d..1fc7221c 100644
--- a/lib/sbi/sbi_sse.c
+++ b/lib/sbi/sbi_sse.c
@@ -1059,25 +1059,39 @@ 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];
+
+	/*
+	 * The caller rejects a larger count, but local_attrs is a fixed
+	 * size stack array, so do not depend on a check made elsewhere.
+	 */
+	if (attr_count > SBI_SSE_ATTR_MAX)
+		return SBI_ERR_INVALID_PARAM;
 
 	sbi_hart_protection_temp_map_range(input_phys, sizeof(unsigned long) * attr_count);
 
+	/*
+	 * Snapshot the attributes so that the values checked below are the
+	 * ones applied, even though S-mode can keep writing the shared
+	 * memory.
+	 */
+	copy_attrs(local_attrs, attrs, attr_count);
+
+	sbi_hart_protection_temp_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_temp_unmap_range(input_phys, sizeof(unsigned long) * attr_count);
-
 	return ret;
 }
 
-- 
2.34.1