Re: [PATCH v7 23/23] [RFC] tools/scmi: Add SCMI Telemetry testing tool

Hi Cristian,

A few issues remain in the test tool inline.

> 	st = malloc(sizeof(*st));
> 	if (!st)
> 		return NULL;
>
> 	st->fd = open(path, O_RDWR);

st is still uninitialized. Members populated only for nonzero optional
resources can therefore contain arbitrary pointers.

Please use calloc() or explicitly zero the complete structure before
assigning fields.

> 	if (st->info.num_groups)
> 		st->grps = enumerate_groups(st);
>
> 	if (st->info.num_shmtis)
> 		st->ssl = enumerate_shmtis(st);

These optional pointers remain uninitialized when their resource counts
are zero. This is particularly problematic because later display and
command paths dereference them without consistently checking the count
and pointer.

> 	fprintf(stdout, "\n+ Found #%u SHMTI areas\n",
> 		st->ssl->num_shmtis);
> 	shinfo = (struct scmi_tlm_shmti_info *)st->ssl->shmtis;

st->ssl is only populated when num_shmtis is nonzero. A valid platform
with no SHMTIs dereferences an uninitialized or NULL pointer here. Please
guard this entire block using both the resource count and a valid pointer.

> 		grp->ivs = enumerate_intervals(st->fd,
> 					       grp->info->num_intervals,
> 					       &grp->info->grp_id);

This still calls the interval ioctl when num_intervals is zero. Please
leave grp->ivs as NULL unless per-group interval configuration is
supported and the reported count is nonzero.

dump_groups() must also handle a NULL grp->ivs rather than dereferencing
it unconditionally.

> 	tdcf = mmap(NULL, shmti->len, PROT_READ, MAP_SHARED,
> 		    shmti->fd, 0);
> [...]
> 	tdcf += shmti->offset;

The mapping length must include the SHMTI offset and page rounding.
Mapping only len bytes and then accessing offset + len can cross the VMA.

Please map PAGE_ALIGN(offset + len), preserve the original mapping base,
and munmap() the complete range before returning.

> 	do {
> 		bytes += write(1, tdcf + bytes, shmti->len - bytes);
> 		if (bytes < 0)
> 			return -1;
> 	} while (bytes < shmti->len);

Please store the write() result separately and handle -1 and zero before
adding it to bytes. Adding -1 to an already positive total can leave bytes
nonnegative and miss the error.

This error path should also unmap the mapping before returning.

> 		tdcf = mmap(NULL, shmtis[i].len, PROT_READ, MAP_SHARED,
> 			    shmtis[i].fd, 0);
> [...]
> 		start = tdcf + shmtis[i].offset;
> 		end = tdcf + shmtis[i].offset + shmtis[i].len;

The same offset-aware mapping-length problem exists here, and none of
these mappings are unmapped. Please preserve each mapping base and call
munmap() on every success and error path.

> 	batch = batch_buffer_alloc(st->info.num_des,
> 				   sizeof(*samples), true);
> [...]
> 	batch->num_items = args->cnt;
> 	for (int i = 0; i < batch->num_items; i++) {
> 		unsigned long val;
>
> 		val = strtoul(args->opts[i], NULL, 0);
> [...]
> 		samples[i].id = (unsigned int)val;
> 	}

The allocation is sized using num_des, but num_items is then replaced with
args->cnt and that many entries are written. Supplying more command-line
IDs than platform DEs writes beyond the allocation.

Please allocate using the validated argument count or reject counts above
the supported limit.

> 	if (need_status) {
> 		int *states;
> 		size_t states_sz = sizeof(*states) * batch->num_items;
>
> 		states = malloc(states_sz);
> 		if (!states)
> 			return NULL;

If status allocation fails, the already allocated item buffer and batch
wrapper are leaked. Please unwind both preceding allocations here.

The size calculations in this helper should also use checked
multiplication.

> 	ret = gather_tlm_state(st);
> 	if (ret) {
> 		free(st);
> 		return ret;
> 	}
> [...]
> 	return 0;

There is still no complete session cleanup. The main device fd, returned
SHMTI fds, mappings, interval arrays, DE/group descriptors, batch buffers
and nested allocations remain open or allocated on normal and error
exits.

Please add one cleanup function and route all exits through it. Apart from
fixing the leaks, that would demonstrate the intended lifetime and cleanup
requirements of the UAPI.

Thanks,
Fayçal