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

Hi Cristian,=0A=
=0A=
Thanks for addressing the concrete V5 issues. I found a few remaining=0A=
implementation problems inline.=0A=
=0A=
> +	u8 *impl_version =3D (u8 *)in->base.de_impl_version;=0A=
> [...]=0A=
> +	/* Each word composing the UUID was stored in big-endian */=0A=
> +	for (int i =3D 0; i < SCMI_TLM_DE_IMPL_UUID_MAX; i++)=0A=
> +		out->de_impl_version[i] =3D impl_version[i];=0A=
=0A=
The internal UUID is stored as CPU-endian u32 values after le32_to_cpu().=
=0A=
Reinterpreting that storage as bytes therefore produces different output=0A=
on little- and big-endian systems.=0A=
=0A=
Please serialize each word explicitly in the byte order required by the=0A=
SCMI specification and document the byte-array representation in the=0A=
UAPI.=0A=
=0A=
> +	if (get_user(usize, (u32 __user *)arg))=0A=
> +		return -EFAULT;=0A=
> +=0A=
> +	if (usize < offsetofend(struct scmi_tlm_abi_info, reserved))=0A=
> +		return -EINVAL;=0A=
> +=0A=
> +	if (copy_struct_from_user(&base, sizeof(base), uptr, usize))=0A=
> +		return -EFAULT;=0A=
=0A=
usize should have an upper bound, conventionally PAGE_SIZE. Otherwise a=0A=
large value can cause copy_struct_from_user() to inspect, and=0A=
copy_struct_to_user() to clear, a very large userspace range.=0A=
=0A=
The error from copy_struct_from_user() should also be propagated. It can=0A=
return -E2BIG for nonzero unknown trailing fields, which is currently=0A=
converted to -EFAULT.=0A=
=0A=
> +		ret =3D tsp->ops->state_set(tsp->ph, true, cfg.grp_id,=0A=
> +					  &ena, &t_ena);=0A=
> +		if (ret)=0A=
> +			return ret;=0A=
> +	}=0A=
> +=0A=
> +	active =3D SCMI_TLM_BUILD_UPDATE_INTERVAL(cfg.active.secs,=0A=
> +						cfg.active.exp);=0A=
> +=0A=
> +	return tsp->ops->collection_configure(...);=0A=
=0A=
There are two issues here:=0A=
=0A=
1. secs and exp are packed without checking that they fit the SCMI=0A=
   encoding. Out-of-range values are silently truncated into a different=0A=
   interval. Please validate both fields, and preferably confirm that the=
=0A=
   result is one of the advertised intervals.=0A=
=0A=
2. For a group, state_set() can succeed and collection_configure() can=0A=
   subsequently fail, leaving partial state. The operation either needs=0A=
   rollback or documented partial-failure semantics.=0A=
=0A=
> +		tlm_ivs =3D rinfo->grps[ivs.grp_id].intervals;=0A=
> +	}=0A=
> +=0A=
> +	if (ivs.num_intervals < tlm_ivs->num_intervals)=0A=
> +		return -ENOSPC;=0A=
=0A=
The group interval pointer can be NULL when per-group configuration is=0A=
unsupported. It may also be NULL following partial group enumeration.=0A=
This then dereferences NULL. Please return -EOPNOTSUPP or the relevant=0A=
enumeration error before accessing it.=0A=
=0A=
Also, scmi_tlm_to_uapi_intervals() does not set=0A=
SCMI_TLM_INTERV_DISCRETE from tlm_ivs->discrete. The returned flag is=0A=
currently inherited from userspace input rather than describing the=0A=
firmware data.=0A=
=0A=
> +	ivs_intrv_sz =3D ivs.num_intervals *=0A=
> +			sizeof(struct scmi_tlm_update_interval);=0A=
> +	struct scmi_tlm_update_interval *ivs_intrv __free(kfree) =3D=0A=
> +		kzalloc(ivs_intrv_sz, GFP_KERNEL);=0A=
=0A=
This multiplication is still unchecked. On a 32-bit kernel, a large=0A=
userspace count can wrap to a small allocation. The subsequent conversion=
=0A=
loops over the real firmware count and can then write beyond that=0A=
allocation.=0A=
=0A=
The same pattern occurs in:=0A=
=0A=
- scmi_tlm_des_list_get_ioctl()=0A=
- scmi_tlm_grp_desc_get_ioctl()=0A=
- scmi_tlm_grps_list_get_ioctl()=0A=
- scmi_tlm_shmtis_list_get_ioctl()=0A=
=0A=
Please allocate with kcalloc()/size_mul() using the required firmware=0A=
count, rather than the possibly oversized userspace capacity. Only the=0A=
actual returned count needs to be copied.=0A=
=0A=
> +	rinfo =3D scmi_telemetry_res_info_get(tsp);=0A=
> +	for (int i =3D 0; i < rinfo->num_des; i++) {=0A=
> +		ret =3D tsp->ops->state_set(tsp->ph, false,=0A=
> +					  rinfo->des[i]->info->id,=0A=
> +					  &ena, &t_ena);=0A=
> +		if (ret)=0A=
> +			return ret;=0A=
> +	}=0A=
=0A=
SET_ALL_CFG still leaves partial configuration if one DE fails. A=0A=
subsequent GET_ALL_CFG only reports a cumulative boolean and does not tell=
=0A=
userspace which DE failed or which earlier changes succeeded.=0A=
=0A=
This needs rollback, or a batch configuration ABI with per-DE status.=0A=
At minimum, the partial-failure behavior and multi-client interaction need=
=0A=
to be documented.=0A=
=0A=
> +	for (int i =3D 0; i < batch.num_samples; i++) {=0A=
> +		int ret;=0A=
> +=0A=
> +		ret =3D tsp->ops->de_data_read(tsp->ph, &samples[i]);=0A=
> +		if (ret)=0A=
> +			return ret;=0A=
> +	}=0A=
=0A=
On failure, successfully read preceding entries are not copied back, and=0A=
there is no indication of which entry failed. This is why I think a=0A=
per-entry status is needed for the batch ABI.=0A=
=0A=
An explicit upper bound for num_samples is also needed to prevent=0A=
unbounded allocation and iteration from an ioctl argument.=0A=
=0A=
> +	if (req < ctx->shmti->len)=0A=
> +		return -EINVAL;=0A=
> +=0A=
> +	base =3D ctx->shmti->phys & PAGE_MASK;=0A=
> +	needed =3D ctx->shmti->len + ctx->shmti->offset;=0A=
> +	expect =3D DIV_ROUND_UP(needed, PAGE_SIZE);=0A=
> +	npages =3D req >> PAGE_SHIFT;=0A=
> +	if (npages > expect)=0A=
> +		return -EINVAL;=0A=
=0A=
The lower-bound check does not include offset. A mapping can therefore be=
=0A=
accepted even when it does not cover offset + len.=0A=
=0A=
Please calculate needed with checked addition and require the VMA to cover=
=0A=
PAGE_ALIGN(needed). The handling of vm_pgoff should also be explicit;=0A=
unless partial mappings are part of the ABI, nonzero vm_pgoff should be=0A=
rejected.=0A=
=0A=
It would also be prudent to reject executable mappings and document=0A=
whether MAP_SHARED is required.=0A=
=0A=
More fundamentally, mapping an unaligned physical area exposes the bytes=0A=
before and after the SHMTI in the same physical pages. The driver cannot=0A=
enforce access only to offset..offset+len at page granularity. The ABI=0A=
therefore needs a platform requirement that these pages contain no other=0A=
sensitive data, or SHMTI mmap should be limited to page-aligned,=0A=
page-exclusive areas.=0A=
=0A=
> +		fd =3D anon_inode_getfd(SCMI_TLM_DRIVER,=0A=
> +				      &scmi_tlm_shmti_fops, ctx,=0A=
> +				      O_RDONLY | O_CLOEXEC);=0A=
> [...]=0A=
> +	scmi_tlm_allocate_anon_fds(ti, &ssl, shinfo);=0A=
> +	if (copy_to_user(u64_to_user_ptr(ssl.shmtis), shinfo, shinfo_sz))=0A=
> +		return -EFAULT;=0A=
> +=0A=
> +	if (copy_to_user(uptr, &ssl, sizeof(ssl)))=0A=
> +		return -EFAULT;=0A=
=0A=
The file descriptors have already been installed when either copyout can=0A=
fail. Returning -EFAULT then leaves installed descriptors that userspace=0A=
does not know about.=0A=
=0A=
Please reserve/build the files first, perform the copyout, and only install=
=0A=
them after all user copies succeed, with rollback for every failure path.=
=0A=
=0A=
The SHMTI context kref only protects the context itself. It does not=0A=
visibly hold a reference to the protocol-owned shmti descriptor or the=0A=
SCMI device. Please clarify how these remain valid across driver unbind=0A=
while the anonymous fd or VMA is still open.=0A=
=0A=
> +static const struct file_operations stlm_fops =3D {=0A=
> +	.owner =3D THIS_MODULE,=0A=
> +	.open =3D nonseekable_open,=0A=
> +	.unlocked_ioctl =3D scmi_tlm_unlocked_ioctl,=0A=
> +	.compat_ioctl =3D compat_ptr_ioctl,=0A=
> +};=0A=
=0A=
An O_RDONLY open can still issue SET_CFG, SET_DE_CFG, SET_ALL_CFG, and=0A=
RESET. Because these mutate global firmware state, should mutating ioctls=
=0A=
at least require FMODE_WRITE?=0A=
=0A=
There is also no per-instance lock around configuration/reset sequences.=0A=
SCMI transactions may be serialized individually, but SET_ALL_CFG and the=
=0A=
two-step group configuration can still interleave with another client.=0A=
That does not guarantee a well-defined "last writer wins" result.=0A=
=0A=
> +	device_destroy(&stlm_class, ti->devt);=0A=
> +	cdev_del(&ti->cdev);=0A=
=0A=
cdev_del() prevents new opens but existing file descriptions can continue=
=0A=
calling the file operations. ti is devm-owned by the SCMI device, so it=0A=
appears that an existing fd can use freed instance/protocol data after=0A=
unbind.=0A=
=0A=
Could the open path take an explicit instance/device reference and release=
=0A=
it from .release?=0A=
=0A=
> +	ret =3D class_register(&stlm_class);=0A=
> +	if (ret)=0A=
> +		return ret;=0A=
> +=0A=
> +	return scmi_register(&scmi_telemetry_driver);=0A=
=0A=
The chrdev region is leaked if class_register() fails. Both the class and=
=0A=
chrdev region are leaked if scmi_register() fails. Please add the=0A=
corresponding unwind paths.=0A=
=0A=
Thanks,=0A=
Fay=E7al=