Re: [PATCH v7 08/23] firmware: arm_scmi: Add Telemetry configuration operations

Hi Cristian,

I found two configuration issues inline.

> 	if (!is_group) {
> 		active_update_interval =
> 			&ti->info.active_update_interval;
> 		current_mode = &ti->info.current_mode;
> 	} else {
> 		struct scmi_telemetry_res_info *rinfo;
>
> 		rinfo = ti->res_get(ti);
> 		active_update_interval =
> 			&rinfo->grps[res_id].active_update_interval;
> 		current_mode = &rinfo->grps[res_id].current_mode;
> 	}
> [...]
> 	if (!ret) {
> 		ti->info.enabled = tlm_enable;
> 		*current_mode = next_mode;
> 		ti->info.notif_enabled =
> 			*current_mode == SCMI_TLM_NOTIFICATION;
> 		if (update_interval_ms)
> 			*active_update_interval =
> 				le32_to_cpu(interval);
> 	}

For a group operation, active_update_interval and current_mode point to the
group, but enabled and notif_enabled are still written into the global
ti->info state.

Disabling one group can therefore make the driver believe the entire
Telemetry instance is disabled, causing reads for unrelated DEs to return
no data. A group's collection mode can similarly overwrite the global
notification state.

Please select group-versus-instance cached state consistently for every
field updated here.

> 			de_offs = le32_to_cpu(resp->shmti_de_offset);
>
> 			shmti = &ti->shmti[sid];
> 			payld = shmti->base;
> 			/* Check boundary first... */
> 			if (de_offs + LINE_LENGTH_BYTES(payld) >=
> 			    shmti->info.len)
> 				return -EPROTO;

The addition can wrap, and the payload must end before the TDCF epilogue,
not merely before the end of the entire SHMTI allocation.

Please use checked addition and verify the complete line against the
payload boundary, such as len - TDCF_EPLG_SZ. The same boundary rule
should be used for an explicitly returned block-timestamp offset.

Thanks,
Fayçal