Re: [PATCH v6 22/22] [RFC] tools/scmi: Add SCMI Telemetry testing tool

[Fayssal Benmlih]

Hi Cristian,

A few issues in the test tool inline.

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

st is not initialized. Several members are only assigned when the platform
reports nonzero resources, but later code assumes the pointers are either
valid or NULL.

Please use calloc() or explicitly zero the structure.

> +	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 initialized when num_shmtis is nonzero. A valid platform
with no SHMTIs can therefore dereference an uninitialized pointer here.

The same type of guard is needed for optional group interval information
when per-group configuration is unsupported.

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

This calls the group interval ioctl even when num_intervals is zero.
Apart from malloc(0) being implementation-dependent, the kernel currently
has no valid group interval table when per-group configuration is
unsupported. Please skip this operation unless the feature and count
indicate that it is supported.

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

The mapping length does not include shmti->offset, but the code accesses
offset + len bytes. If that crosses an additional page, this reads beyond
the VMA.

Please calculate the page-rounded mapping size from offset + len,
preserve the original mapping base, and munmap() that complete range
before returning.

The write result should also be stored separately. Adding -1 to a
previously positive byte count does not necessarily make bytes negative,
so the current error check can miss failures.

> +		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 mapping-length issue exists here, and none of these mappings are
unmapped.

> +	if (ret) {
> +		free(st);
> +		return ret;
> +	}

This only frees the top-level state. The main device fd, returned SHMTI
fds, arrays, group descriptors, intervals, and mappings are not released.

Even as an RFC test tool, a single cleanup function would make error-path
testing reliable and would demonstrate the expected UAPI lifetime
semantics.

Thanks,
Fayçal