Re: [PATCH v7 20/23] firmware: arm_scmi: Add System Telemetry driver

Hi Cristian,

Thanks for adding the batch status and write-mode support. I found a
number of remaining implementation issues inline.

> 	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 still has no upper bound, so a large value can make the struct
helpers inspect or clear a very large userspace range. Please reject
values above a conventional limit such as PAGE_SIZE.

Please also propagate the actual copy_struct_from_user() error instead of
converting errors such as -E2BIG into -EFAULT.

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

secs and exp are still packed without checking that they fit the SCMI
interval encoding. Out-of-range values can therefore be silently
truncated into a different interval. Please validate both fields and
preferably verify that the encoded value belongs to the advertised
discrete or segmented interval set.

For a group, state_set() can also succeed before
collection_configure() fails, leaving partially applied state. Please add
rollback or define the partial-failure behavior. A per-instance lock is
also needed if this multi-command operation must not interleave with
another caller.

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

rinfo->grps[ivs.grp_id].intervals can be NULL when per-group interval
configuration is unsupported or enumeration was incomplete. This is
dereferenced immediately afterward. Please return -EOPNOTSUPP or the
saved enumeration error before accessing it.

> 	out->flags |= in->discrete ? SCMI_TLM_INTERV_DISCRETE : 0;

Using |= does not fully fix the input-inheritance issue. If userspace
supplies SCMI_TLM_INTERV_DISCRETE and firmware reports segmented
intervals, the bit remains set.

Please clear the output-only bit first and then set it solely from
in->discrete.

> 	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 remains unchecked and the allocation still uses the
userspace capacity instead of the required firmware count. On a 32-bit
kernel the size can wrap before kzalloc().

Please use kcalloc() or size_mul() with tlm_ivs->num_intervals and copy
only the number of entries actually returned. The same pattern remains in
the other variable-length enumeration handlers.

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

SET_ALL_CFG still leaves preceding DEs modified if a later state_set()
fails. GET_ALL_CFG only returns cumulative booleans, so userspace cannot
determine what changed.

Please provide rollback or define/report partial completion, and serialize
this sequence against other configuration and reset callers.

> 	if (batch.states) {
> 		int *states_arr __free(kfree) =
> 			kcalloc(batch.num_items, sizeof(*states_arr),
> 				GFP_KERNEL);
> [...]
> 		if (copy_from_user(states_arr,
> 				   u64_to_user_ptr(batch.states),
> 				   batch.num_items * sizeof(*states_arr)))
> 			return -EFAULT;
>
> 		states = no_free_ptr(states_arr);
> 	}

states is documented as an output array. Copying its initial contents from
userspace means successful entries retain arbitrary user-provided values,
because only failures assign states[i].

Please leave the kcalloc()-initialized zeros in place and only store a
negative status for failed entries. The same issue exists in the DE
configuration get and batch-read handlers.

There is also a cleanup issue after no_free_ptr(): several subsequent
copy_to_user() failures return before the manual kfree(states), leaking
the array. Please retain automatic cleanup through every copyout path or
use a common cleanup label.

> 	struct scmi_telemetry_de_sample *samples __free(kfree) =
> 		kcalloc(batch.num_items, sizeof(*samples), GFP_KERNEL);

There is still no explicit maximum for batch.num_items. Please validate it
against an ABI-defined limit before allocating or iterating.

> 	/* Check that the requested size is acceptable */
> 	if (udl.num_uuids > ti->info->num_uuids)
> 		return -ENOSPC;

This comparison appears reversed. A capacity larger than the available
UUID count should be accepted, while a smaller nonzero capacity should
return -ENOSPC and report the required count.

The current code rejects oversized buffers and silently returns only a
prefix for undersized buffers.

> 	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 required mapping length is PAGE_ALIGN(offset + len), not just len. A
one-page request can still be accepted when offset + len crosses into a
second page.

Please calculate needed using checked addition and require the VMA to
cover the page-rounded result. Please also reject nonzero vm_pgoff unless
partial mappings are intentionally part of the ABI.

Should executable and private mappings also be rejected? The documented
interface only appears to require a read-only shared mapping.

> 		fd = anon_inode_getfd(SCMI_TLM_DRIVER,
> 				      &scmi_tlm_shmti_fops, ctx,
> 				      O_RDONLY | O_CLOEXEC);

anon_inode_getfd() installs the descriptor immediately, but both
subsequent userspace copies can fail. Returning -EFAULT then leaves
installed fds that userspace does not know about.

Please reserve and build the files first, perform all copyouts, and only
call fd_install() after they succeed, with rollback for every failure
path.

> struct scmi_tlm_shmti_ctx {
> 	struct scmi_telemetry_shmti_info *shmti;
> 	struct kref kref;
> };

The kref only protects this context; it does not pin the protocol-owned
SHMTI descriptor or the SCMI device. How are ctx->shmti and its physical
metadata kept valid across driver unbind while an anonymous fd or VMA
remains 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,
> };

Existing device fds can outlive cdev_del(), but every ioctl obtains ti
from the embedded cdev and ti is devm-owned by the SCMI device. This
appears to permit use-after-free after unbind.

Could open create a per-file context that pins the instance/device, with
release dropping that reference and rejecting operations once removal
begins?

> 	ret = xa_alloc(&ti->events_xa, &cookie, ctx,
> 		       scmi_evt_xa_limit, GFP_KERNEL);

Event subscriptions are stored instance-wide rather than per open file,
and stlm_fops has no release callback. Closing the telemetry fd therefore
leaves the eventfd reference and firmware subscription active until
explicit cancellation or device removal.

It also allows another opener that knows the cookie to cancel the
subscription. Please associate subscriptions with an open-file context
and clean them up automatically from release.

Thanks,
Fayçal