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

Hi Cristian,=0A=
=0A=
A few issues in the test tool inline.=0A=
=0A=
> +	st =3D malloc(sizeof(*st));=0A=
> +	if (!st)=0A=
> +		return NULL;=0A=
> +=0A=
> +	st->fd =3D open(path, O_RDWR);=0A=
=0A=
st is not initialized. Several members are only assigned when the platform=
=0A=
reports nonzero resources, but later code assumes the pointers are either=
=0A=
valid or NULL.=0A=
=0A=
Please use calloc() or explicitly zero the structure.=0A=
=0A=
> +	fprintf(stdout, "\n+ Found #%u SHMTI areas\n",=0A=
> +		st->ssl->num_shmtis);=0A=
> +	shinfo =3D (struct scmi_tlm_shmti_info *)st->ssl->shmtis;=0A=
=0A=
st->ssl is only initialized when num_shmtis is nonzero. A valid platform=0A=
with no SHMTIs can therefore dereference an uninitialized pointer here.=0A=
=0A=
The same type of guard is needed for optional group interval information=0A=
when per-group configuration is unsupported.=0A=
=0A=
> +		grp->ivs =3D enumerate_intervals(st->fd,=0A=
> +					       grp->info->num_intervals,=0A=
> +					       &grp->info->grp_id);=0A=
=0A=
This calls the group interval ioctl even when num_intervals is zero.=0A=
Apart from malloc(0) being implementation-dependent, the kernel currently=
=0A=
has no valid group interval table when per-group configuration is=0A=
unsupported. Please skip this operation unless the feature and count=0A=
indicate that it is supported.=0A=
=0A=
> +	tdcf =3D mmap(NULL, shmti->len, PROT_READ, MAP_SHARED, shmti->fd, 0);=
=0A=
> [...]=0A=
> +	tdcf +=3D shmti->offset;=0A=
> +	do {=0A=
> +		bytes +=3D write(1, tdcf + bytes, shmti->len - bytes);=0A=
=0A=
The mapping length does not include shmti->offset, but the code accesses=0A=
offset + len bytes. If that crosses an additional page, this reads beyond=
=0A=
the VMA.=0A=
=0A=
Please calculate the page-rounded mapping size from offset + len,=0A=
preserve the original mapping base, and munmap() that complete range=0A=
before returning.=0A=
=0A=
The write result should also be stored separately. Adding -1 to a=0A=
previously positive byte count does not necessarily make bytes negative,=0A=
so the current error check can miss failures.=0A=
=0A=
> +		tdcf =3D mmap(NULL, shmtis[i].len, PROT_READ, MAP_SHARED,=0A=
> +			    shmtis[i].fd, 0);=0A=
> [...]=0A=
> +		start =3D tdcf + shmtis[i].offset;=0A=
> +		end =3D tdcf + shmtis[i].offset + shmtis[i].len;=0A=
=0A=
The same mapping-length issue exists here, and none of these mappings are=
=0A=
unmapped.=0A=
=0A=
> +	if (ret) {=0A=
> +		free(st);=0A=
> +		return ret;=0A=
> +	}=0A=
=0A=
This only frees the top-level state. The main device fd, returned SHMTI=0A=
fds, arrays, group descriptors, intervals, and mappings are not released.=
=0A=
=0A=
Even as an RFC test tool, a single cleanup function would make error-path=
=0A=
testing reliable and would demonstrate the expected UAPI lifetime=0A=
semantics.=0A=
=0A=
Thanks,=0A=
Fay=E7al=