Re: [PATCH v10 5/8] coresight: etm4x: introduce struct etm4_caps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Hardware-reported capability counts are used as loop limits without being checked against static array capacities, leading to heap buffer overflows.
--

commit 1fb7a47435872f1b7f11824178d098d0258effdc
Author: Yeoreum Yun <yeoreum.yun@arm.com>

coresight: etm4x: introduce struct etm4_caps

This commit introduces struct etmv4_caps to encapsulate ETMv4 capabilities 
and migrates the relevant capability information out of struct etmv4_drvdata 
into it.

> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
[ ... ]
> @@ -1491,43 +1501,43 @@ static void etm4_init_arch_data(void *info)
[ ... ]
> -	drvdata->nr_ss_cmp = FIELD_GET(TRCIDR4_NUMSSCC_MASK, etmidr4);
> -	for (i = 0; i < drvdata->nr_ss_cmp; i++) {
> +	caps->nr_ss_cmp = FIELD_GET(TRCIDR4_NUMSSCC_MASK, etmidr4);
> +	for (i = 0; i < caps->nr_ss_cmp; i++) {
>  		drvdata->config.ss_status[i] =
>  			etm4x_relaxed_read32(csa, TRCSSCSRn(i));
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this code overflow the ss_status[] 
array if the hardware reports a capability count greater than the static array 
capacity?

In etm4_init_arch_data(), caps->nr_ss_cmp is read directly from the TRCIDR4 
hardware register. The architecture allows this field to be up to 15, but the 
arrays like ss_status are fixed in size to ETM_MAX_SS_CMP. Using the 
unvalidated hardware capability as a loop limit can lead to a heap buffer 
overflow.

[ ... ]
> @@ -1892,57 +1903,56 @@ static int etm4_cpu_save(struct coresight_device *csdev)
[ ... ]
> -	for (i = 0; i < drvdata->nr_ss_cmp; i++) {
> +	for (i = 0; i < caps->nr_ss_cmp; i++) {
>  		state->trcssccr[i] = etm4x_read32(csa, TRCSSCCRn(i));
>  		state->trcsscsr[i] = etm4x_read32(csa, TRCSSCSRn(i));
>  		if (etm4x_sspcicrn_present(drvdata, i))
>  			state->trcsspcicr[i] = etm4x_read32(csa, TRCSSPCICRn(i));
>  	}

[Severity: Critical]
This isn't a bug introduced by this patch, but does this loop risk corrupting
the dynamically allocated etmv4_save_state structure? 

Similar to the initialization path, etm4_cpu_save() relies on the unvalidated 
caps->nr_ss_cmp bound to write into state->trcssccr[] and state->trcsscsr[]. 
If the hypervisor or hardware provides a larger count, this will overflow the 
save state buffers on suspend and resume paths.

[ ... ]
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
[ ... ]
> @@ -2059,7 +2090,7 @@ static ssize_t ctxid_masks_store(struct device *dev,
[ ... ]
>  	mask = config->ctxid_mask0;
> -	for (i = 0; i < drvdata->numcidc; i++) {
> +	for (i = 0; i < caps->numcidc; i++) {
>  		/* mask value of corresponding ctxid comparator */
>  		maskbyte = mask & ETMv4_EVENT_MASK;
>  		/*
>  		 * each bit corresponds to a byte of respective ctxid comparator
>  		 * value register
>  		 */
>  		for (j = 0; j < 8; j++) {
>  			if (maskbyte & 1)
>  				config->ctxid_pid[i] &= ~(0xFFUL << (j * 8));

[Severity: Critical]
This is a pre-existing issue, but could this sysfs-triggered write also 
overflow the ctxid_pid[] array?

In ctxid_masks_store(), caps->numcidc is used as the loop bound for updating 
config->ctxid_pid[i], which is dimensioned by ETMv4_MAX_CTXID_CMP. Without 
clamping the hardware-reported capability, writing to this sysfs node could 
result in out-of-bounds memory accesses.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-separate_etm_cfg_v2-v10-0-1b715d95927a@arm.com?part=5