Re: [PATCH v6 19/22] firmware: arm_scmi: Add System Telemetry driver

[Fayssal Benmlih]

Hi Cristian,

Thanks for addressing the concrete V5 issues. I found a few remaining
implementation problems inline.

> +	u8 *impl_version = (u8 *)in->base.de_impl_version;
> [...]
> +	/* Each word composing the UUID was stored in big-endian */
> +	for (int i = 0; i < SCMI_TLM_DE_IMPL_UUID_MAX; i++)
> +		out->de_impl_version[i] = impl_version[i];

The internal UUID is stored as CPU-endian u32 values after le32_to_cpu().
Reinterpreting that storage as bytes therefore produces different output
on little- and big-endian systems.

Please serialize each word explicitly in the byte order required by the
SCMI specification and document the byte-array representation in the
UAPI.

> +	if (get_user(usize, (u32 __user *)arg))
> +		return -EFAULT;
> +
> +	if (usize < offsetofend(struct scmi_tlm_abi_info, reserved))
> +		return -EINVAL;
> +
> +	if (copy_struct_from_user(&base, sizeof(base), uptr, usize))
> +		return -EFAULT;

usize should have an upper bound, conventionally PAGE_SIZE. Otherwise a
large value can cause copy_struct_from_user() to inspect, and
copy_struct_to_user() to clear, a very large userspace range.

The error from copy_struct_from_user() should also be propagated. It can
return -E2BIG for nonzero unknown trailing fields, which is currently
converted to -EFAULT.

> +		ret = tsp->ops->state_set(tsp->ph, true, cfg.grp_id,
> +					  &ena, &t_ena);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	active = SCMI_TLM_BUILD_UPDATE_INTERVAL(cfg.active.secs,
> +						cfg.active.exp);
> +
> +	return tsp->ops->collection_configure(...);

There are two issues here:

1. secs and exp are packed without checking that they fit the SCMI
   encoding. Out-of-range values are silently truncated into a different
   interval. Please validate both fields, and preferably confirm that the
   result is one of the advertised intervals.

2. For a group, state_set() can succeed and collection_configure() can
   subsequently fail, leaving partial state. The operation either needs
   rollback or documented partial-failure semantics.

> +		tlm_ivs = rinfo->grps[ivs.grp_id].intervals;
> +	}
> +
> +	if (ivs.num_intervals < tlm_ivs->num_intervals)
> +		return -ENOSPC;

The group interval pointer can be NULL when per-group configuration is
unsupported. It may also be NULL following partial group enumeration.
This then dereferences NULL. Please return -EOPNOTSUPP or the relevant
enumeration error before accessing it.

Also, scmi_tlm_to_uapi_intervals() does not set
SCMI_TLM_INTERV_DISCRETE from tlm_ivs->discrete. The returned flag is
currently inherited from userspace input rather than describing the
firmware data.

> +	ivs_intrv_sz = ivs.num_intervals *
> +			sizeof(struct scmi_tlm_update_interval);
> +	struct scmi_tlm_update_interval *ivs_intrv __free(kfree) =
> +		kzalloc(ivs_intrv_sz, GFP_KERNEL);

This multiplication is still unchecked. On a 32-bit kernel, a large
userspace count can wrap to a small allocation. The subsequent conversion
loops over the real firmware count and can then write beyond that
allocation.

The same pattern occurs in:

- scmi_tlm_des_list_get_ioctl()
- scmi_tlm_grp_desc_get_ioctl()
- scmi_tlm_grps_list_get_ioctl()
- scmi_tlm_shmtis_list_get_ioctl()

Please allocate with kcalloc()/size_mul() using the required firmware
count, rather than the possibly oversized userspace capacity. Only the
actual returned count needs to be copied.

> +	rinfo = scmi_telemetry_res_info_get(tsp);
> +	for (int i = 0; i < rinfo->num_des; i++) {
> +		ret = tsp->ops->state_set(tsp->ph, false,
> +					  rinfo->des[i]->info->id,
> +					  &ena, &t_ena);
> +		if (ret)
> +			return ret;
> +	}

SET_ALL_CFG still leaves partial configuration if one DE fails. A
subsequent GET_ALL_CFG only reports a cumulative boolean and does not tell
userspace which DE failed or which earlier changes succeeded.

This needs rollback, or a batch configuration ABI with per-DE status.
At minimum, the partial-failure behavior and multi-client interaction need
to be documented.

> +	for (int i = 0; i < batch.num_samples; i++) {
> +		int ret;
> +
> +		ret = tsp->ops->de_data_read(tsp->ph, &samples[i]);
> +		if (ret)
> +			return ret;
> +	}

On failure, successfully read preceding entries are not copied back, and
there is no indication of which entry failed. This is why I think a
per-entry status is needed for the batch ABI.

An explicit upper bound for num_samples is also needed to prevent
unbounded allocation and iteration from an ioctl argument.

> +	if (req < ctx->shmti->len)
> +		return -EINVAL;
> +
> +	base = ctx->shmti->phys & PAGE_MASK;
> +	needed = ctx->shmti->len + ctx->shmti->offset;
> +	expect = DIV_ROUND_UP(needed, PAGE_SIZE);
> +	npages = req >> PAGE_SHIFT;
> +	if (npages > expect)
> +		return -EINVAL;

The lower-bound check does not include offset. A mapping can therefore be
accepted even when it does not cover offset + len.

Please calculate needed with checked addition and require the VMA to cover
PAGE_ALIGN(needed). The handling of vm_pgoff should also be explicit;
unless partial mappings are part of the ABI, nonzero vm_pgoff should be
rejected.

It would also be prudent to reject executable mappings and document
whether MAP_SHARED is required.

More fundamentally, mapping an unaligned physical area exposes the bytes
before and after the SHMTI in the same physical pages. The driver cannot
enforce access only to offset..offset+len at page granularity. The ABI
therefore needs a platform requirement that these pages contain no other
sensitive data, or SHMTI mmap should be limited to page-aligned,
page-exclusive areas.

> +		fd = anon_inode_getfd(SCMI_TLM_DRIVER,
> +				      &scmi_tlm_shmti_fops, ctx,
> +				      O_RDONLY | O_CLOEXEC);
> [...]
> +	scmi_tlm_allocate_anon_fds(ti, &ssl, shinfo);
> +	if (copy_to_user(u64_to_user_ptr(ssl.shmtis), shinfo, shinfo_sz))
> +		return -EFAULT;
> +
> +	if (copy_to_user(uptr, &ssl, sizeof(ssl)))
> +		return -EFAULT;

The file descriptors have already been installed when either copyout can
fail. Returning -EFAULT then leaves installed descriptors that userspace
does not know about.

Please reserve/build the files first, perform the copyout, and only install
them after all user copies succeed, with rollback for every failure path.

The SHMTI context kref only protects the context itself. It does not
visibly hold a reference to the protocol-owned shmti descriptor or the
SCMI device. Please clarify how these remain valid across driver unbind
while the anonymous fd or VMA is still open.

> +static const struct file_operations stlm_fops = {
> +	.owner = THIS_MODULE,
> +	.open = nonseekable_open,
> +	.unlocked_ioctl = scmi_tlm_unlocked_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
> +};

An O_RDONLY open can still issue SET_CFG, SET_DE_CFG, SET_ALL_CFG, and
RESET. Because these mutate global firmware state, should mutating ioctls
at least require FMODE_WRITE?

There is also no per-instance lock around configuration/reset sequences.
SCMI transactions may be serialized individually, but SET_ALL_CFG and the
two-step group configuration can still interleave with another client.
That does not guarantee a well-defined "last writer wins" result.

> +	device_destroy(&stlm_class, ti->devt);
> +	cdev_del(&ti->cdev);

cdev_del() prevents new opens but existing file descriptions can continue
calling the file operations. ti is devm-owned by the SCMI device, so it
appears that an existing fd can use freed instance/protocol data after
unbind.

Could the open path take an explicit instance/device reference and release
it from .release?

> +	ret = class_register(&stlm_class);
> +	if (ret)
> +		return ret;
> +
> +	return scmi_register(&scmi_telemetry_driver);

The chrdev region is leaked if class_register() fails. Both the class and
chrdev region are leaked if scmi_register() fails. Please add the
corresponding unwind paths.

Thanks,
Fayçal