Re: [PATCH v4 06/31] dt-bindings: firmware: arm,scmi: Add support for telemetry protocol

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] A race condition in `scmi_protocol_release()` against `scmi_get_protocol_instance()` leads to a Use-After-Free of the protocol instance.
- [High] Concurrent requests for the same uninitialized protocol will fail instead of waiting for initialization.
- [Medium] Using `_IOWR` ioctl macros with structures containing flexible array members truncates the encoded payload size.
--

commit c0ccec86311b387c10216dd1ee33220fb6d9c890
Author: Cristian Marussi <cristian.marussi@arm.com>

dt-bindings: firmware: arm,scmi: Add support for telemetry protocol

Add new SCMI v4.0 Telemetry protocol bindings definitions.

> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index f167194f7cf67..9c1ab9925b1d7 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c

[ ... ]

> @@ -2284,27 +2298,25 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
>  static struct scmi_protocol_instance * __must_check
>  scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
>  {
> -	struct scmi_protocol_instance *pi;
>  	struct scmi_info *info = handle_to_scmi_info(handle);
> +	const struct scmi_protocol *proto;
>  
> -	mutex_lock(&info->protocols_mtx);
> -	pi = idr_find(&info->protocols, protocol_id);
> +	scoped_guard(mutex, &info->protocols_mtx) {
> +		struct scmi_protocol_instance *pi;
>  
> -	if (pi) {
> -		refcount_inc(&pi->users);
> -	} else {
> -		const struct scmi_protocol *proto;
> -
> -		/* Fails if protocol not registered on bus */
> -		proto = scmi_protocol_get(protocol_id, &info->version);
> -		if (proto)
> -			pi = scmi_alloc_init_protocol_instance(info, proto);
> -		else
> -			pi = ERR_PTR(-EPROBE_DEFER);
> +		pi = idr_find(&info->protocols, protocol_id);
> +		if (pi) {
> +			refcount_inc(&pi->users);
> +			return pi;
> +		}
>  	}
> -	mutex_unlock(&info->protocols_mtx);
>  
> -	return pi;
> +	/* Fails if protocol not registered on bus */
> +	proto = scmi_protocol_get(protocol_id, &info->version);
> +	if (!proto)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	return scmi_alloc_init_protocol_instance(info, proto);
>  }

[Severity: High]
What happens if two threads concurrently request the same uninitialized
protocol?

Since the mutex is dropped after failing to find the protocol, both threads
would independently call scmi_alloc_init_protocol_instance() and initialize
a new protocol instance. The first thread will successfully call idr_alloc(),
while the second thread's idr_alloc() for the same ID will fail with -ENOSPC.

Would this regression cause the second thread to tear down its allocation and
return an error, leading to a spurious fatal probe failure for concurrent
devices instead of waiting and reusing the initialized instance?

[ ... ]

> @@ -2335,10 +2347,11 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
>  	struct scmi_info *info = handle_to_scmi_info(handle);
>  	struct scmi_protocol_instance *pi;
>  
> -	mutex_lock(&info->protocols_mtx);
> -	pi = idr_find(&info->protocols, protocol_id);
> -	if (WARN_ON(!pi))
> -		goto out;
> +	scoped_guard(mutex, &info->protocols_mtx) {
> +		pi = idr_find(&info->protocols, protocol_id);
> +		if (WARN_ON(!pi))
> +			return;
> +	}
>  
>  	if (refcount_dec_and_test(&pi->users)) {

[Severity: Critical]
Could dropping the protocols_mtx lock before calling refcount_dec_and_test()
introduce a use-after-free regression?

If thread 1 reaches this point and is about to drop the refcount to 0,
another thread calling scmi_get_protocol_instance() might acquire the lock,
find the same instance in the IDR, and call refcount_inc(&pi->users) on it
while its refcount is zero.

Once thread 1 re-acquires the lock to remove the instance via idr_remove()
and frees it, thread 2 would be left with a dangling pointer.

[ ... ]

> diff --git a/include/uapi/linux/scmi.h b/include/uapi/linux/scmi.h
> new file mode 100644
> index 0000000000000..abf68bb999606
> --- /dev/null
> +++ b/include/uapi/linux/scmi.h

[ ... ]

> +struct scmi_tlm_intervals {
> +	__u8 discrete;
> +	__u8 pad[3];
> +	__u32 num_intervals;
> +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_LOW	0
> +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_HIGH	1
> +#define SCMI_TLM_UPDATE_INTVL_SEGMENT_STEP	2
> +	__u32 update_intervals[] __counted_by(num_intervals);
> +};

[ ... ]

> +#define SCMI_TLM_GET_INTRVS	_IOWR(SCMI, 0x03, struct scmi_tlm_intervals)
> +#define SCMI_TLM_GET_DE_CFG	_IOWR(SCMI, 0x04, struct scmi_tlm_de_config)
> +#define SCMI_TLM_SET_DE_CFG	_IOWR(SCMI, 0x05, struct scmi_tlm_de_config)
> +#define SCMI_TLM_GET_DE_INFO	_IOWR(SCMI, 0x06, struct scmi_tlm_de_info)
> +#define SCMI_TLM_GET_DE_LIST	_IOWR(SCMI, 0x07, struct scmi_tlm_des_list)
> +#define SCMI_TLM_GET_DE_VALUE	_IOWR(SCMI, 0x08, struct scmi_tlm_de_sample)
> +#define SCMI_TLM_SET_ALL_CFG	_IOW(SCMI,  0x09, struct scmi_tlm_de_config)
> +#define SCMI_TLM_GET_GRP_LIST	_IOWR(SCMI, 0x0A, struct scmi_tlm_grps_list)
> +#define SCMI_TLM_GET_GRP_INFO	_IOR(SCMI,  0x0B, struct scmi_tlm_grp_info)
> +#define SCMI_TLM_GET_GRP_DESC	_IOWR(SCMI, 0x0C, struct scmi_tlm_grp_desc)
> +#define SCMI_TLM_SINGLE_SAMPLE	_IOWR(SCMI, 0x0D, struct scmi_tlm_data_read)
> +#define SCMI_TLM_BULK_READ	_IOWR(SCMI, 0x0E, struct scmi_tlm_data_read)

[Severity: Medium]
Does using the _IOWR() macro with structures containing flexible array
members cause regressions with payload size encoding?

The _IOWR() macro computes the copy size using sizeof(type), which only
covers the fixed header bytes and ignores the flexible array elements (like
update_intervals in struct scmi_tlm_intervals). This embeds an artificially
small size into the IOCTL command number.

While the native kernel driver handles memory boundaries explicitly via
copy_from_user lengths, standard system call tracers like strace and
emulators like QEMU user-mode emulation rely on the _IOC_SIZE() macro
embedded in the IOCTL definition to identify the memory payload size. Will
this result in truncating the array copy and causing emulation faults for
IOCTLs like SCMI_TLM_GET_INTRVS, SCMI_TLM_GET_DE_LIST,
SCMI_TLM_SINGLE_SAMPLE, and SCMI_TLM_BULK_READ?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612223802.1337232-1-cristian.marussi@arm.com?part=6