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

[Himanshu Chauhan]

On Thu, Nov 05, 2026 at 01:39:58AM +0000, liutong wrote:
> 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.
> For SBI_SSE_ATTR_PREFERRED_HART this leaves e->hartindex derived from
> a hart id that sse_event_set_hart_id_check() refused, which
> sbi_sse_enable() then resolves to a NULL scratch.
> 
> 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. Check attr_count here as
> well, so that the bound of the array does not depend on validation done
> in another function.
> 
> 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>
> ---
> Resent on its own from "[PATCH v3 0/3] lib: sbi: fix shared memory
> double-fetch in DBTR and SSE":
> 
>   https://lore.kernel.org/opensbi/20260910052958.3365324-4-liutong@iscas.ac.cn/
> 
> Himanshu NAKed that series on a debug trigger argument. This one is a
> different subsystem, so I would rather it be judged on its own; if the
> NAK was meant to cover it too, say so and I will drop it.
>

LGTM. You can drop this agent conversation bit.

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

> The attribute that matters is PREFERRED_HART: winning the race stores a
> hart id that sse_event_set_hart_id_check() refused, e->hartindex becomes
> -1U, and sbi_sse_enable() then dereferences a NULL scratch. Measured on
> f6abdaf2 under QEMU MTTCG, -smp 2: unpatched, won in 6 rounds, and
> SSE_ENABLE then takes an M-mode load access fault (mcause=0x5,
> mtval=0x540); patched, 20000 rounds with no win.
> 
> Unchanged from v3 apart from the commit message, which overstated the
> bound check: it covers attr_count only.
>  lib/sbi/sbi_sse.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/sbi/sbi_sse.c b/lib/sbi/sbi_sse.c
> index 767ea128..b3d35d65 100644
> --- a/lib/sbi/sbi_sse.c
> +++ b/lib/sbi/sbi_sse.c
> @@ -1049,25 +1049,40 @@ 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_sse_attr_check() in the only caller already rejects a larger
> +	 * count, but local_attrs is a fixed size stack array, so do not
> +	 * rely on that for its bound.
> +	 */
> +	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;
>  }
>  
> 
> base-commit: f6abdaf25dfe0608277fc634f3927c1f184ede6d
> -- 
> 2.34.1
>