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

[Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>]

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