Re: [RFC PATCH 3/7] firmware: arm_scmi: Add Telemetry protocol support

[Dan Carpenter <dan.carpenter@linaro.org>]

On Fri, Jun 20, 2025 at 08:28:09PM +0100, Cristian Marussi wrote:
> +static int
> +scmi_telemetry_protocol_attributes_get(const struct scmi_protocol_handle *ph,
> +				       struct telemetry_info *ti)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +	struct scmi_msg_resp_telemetry_protocol_attributes *resp;
> +
> +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES,
> +				      0, sizeof(*resp), &t);
> +	if (ret)
> +		return ret;
> +
> +	resp = t->rx.buf;
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {
> +		__le32 attr = resp->attributes;
> +
> +		ti->info.num_de = le32_to_cpu(resp->de_num);
> +		ti->info.num_groups = le32_to_cpu(resp->groups_num);
> +		for (int i = 0; i < SCMI_TLM_MAX_DWORD; i++)
> +			ti->info.de_impl_version[i] =
> +				le32_to_cpu(resp->de_implementation_rev_dword[i]);
> +		ti->info.single_read_support = SUPPORTS_SINGLE_READ(attr);
> +		ti->info.continuos_update_support = SUPPORTS_CONTINUOS_UPDATE(attr);
> +		ti->info.per_group_config_support = SUPPORTS_PER_GROUP_CONFIG(attr);
> +		ti->info.reset_support = SUPPORTS_RESET(attr);
> +		ti->info.fc_support = SUPPORTS_FC(attr);
> +		ti->num_shmti = le32_get_bits(attr, GENMASK(15, 0));
> +		/* Allocate DEs descriptors */
> +		ti->info.des = devm_kcalloc(ph->dev, ti->info.num_de,
> +					    sizeof(*ti->info.des), GFP_KERNEL);
> +		if (!ti->info.des)
> +			ret = -ENOMEM;
> +
> +		/* Allocate DE GROUPS descriptors */
> +		ti->info.des_groups = devm_kcalloc(ph->dev, ti->info.num_groups,
> +						   sizeof(*ti->info.des_groups),
> +						   GFP_KERNEL);
> +		if (!ti->info.des_groups)
> +			ret = -ENOMEM;

It the allocation fails we need to jump to the ->xfer_put

> +
> +		for (int i = 0; i < ti->info.num_groups; i++)
> +			ti->info.des_groups[i].id = i;

otherwise it leads to a NULL dereference.

> +	}
> +
> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}

[ snip ]

> +static int iter_shmti_process_response(const struct scmi_protocol_handle *ph,
> +				       const void *response,
> +				       struct scmi_iterator_state *st,
> +				       void *priv)
> +{
> +	const struct scmi_msg_resp_telemetry_shmti_list *r = response;
> +	struct telemetry_info *ti = priv;
> +	struct telemetry_shmti *shmti;
> +	const struct scmi_shmti_desc *desc;
> +	void __iomem *addr;
> +	u64 phys_addr;
> +	u32 len;
> +
> +	desc = &r->desc[st->loop_idx];
> +	shmti = &ti->shmti[st->desc_index + st->loop_idx];
> +
> +	shmti->id = le32_to_cpu(desc->id);
> +	phys_addr = le32_to_cpu(desc->addr_low);
> +	phys_addr |= (u64)le32_to_cpu(desc->addr_high) << 32;
> +
> +	len = le32_to_cpu(desc->length);
> +	addr = devm_ioremap(ph->dev, phys_addr, len);
> +	if (!addr)
> +		return -EADDRNOTAVAIL;
> +
> +	shmti->base = addr;
> +	shmti->len = len;

There is some code later which assumes ->len is at least
TDCF_EPLG_SZ and de->data_sz.  This is probably where we should
check if (len < TDCF_EPLG_SZ) return -EINVAL; and the de->data_sz
would be checked later.

> +
> +	return 0;
> +}

regards,
dan carpenter