* Re: [PATCH v4 06/31] dt-bindings: firmware: arm,scmi: Add support for telemetry protocol
2026-06-12 22:37 ` [PATCH v4 06/31] dt-bindings: firmware: arm,scmi: Add support for telemetry protocol Cristian Marussi
@ 2026-06-12 22:53 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-12 22:53 UTC (permalink / raw)
To: Cristian Marussi; +Cc: conor+dt, devicetree, robh
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
^ permalink raw reply [flat|nested] 2+ messages in thread