Re: [PATCH v10 6/8] coresight: etm4x: exclude ss_status from drvdata->config

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Data race on `drvdata->ss_status` between sysfs and concurrent trace sessions.

Pre-existing issues:
- [High] Pre-existing out-of-bounds array access due to missing hardware capability limits check for single-shot comparators and other resources.
- [High] Accessing trace registers after clearing the TRCPDCR_PU power-up bit.
--

commit 3c75fe92e961cdf21f5bd940d7b63e650ac76819
Author: Yeoreum Yun <yeoreum.yun@arm.com>

coresight: etm4x: exclude ss_status from drvdata->config

The purpose of TRCSSCSRn register is to show status of the corresponding
Single-shot Comparator Control and input supports. This patch excludes
ss_status from drvdata->config and moves it directly into drvdata. It also
intentionally preserves existing behavior for single-shot events within the
same session.

> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 6d64a3fbdea4a..68aa99e8135d9 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
[ ... ]
> @@ -573,11 +573,11 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  		etm4x_relaxed_write32(csa, config->res_ctrl[i], TRCRSCTLRn(i));
>  
>  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> -		/* always clear status bit on restart if using single-shot */
> +		/* always clear status and pending bits on restart if using single-shot */
>  		if (config->ss_ctrl[i] || config->ss_pe_cmp[i])
> -			config->ss_status[i] &= ~TRCSSCSRn_STATUS;
> +			drvdata->ss_status[i] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);

[Severity: High]
This is a pre-existing issue, but does this loop in etm4_enable_hw() blindly
rely on the uncapped hardware count caps->nr_ss_cmp? If the hardware reports a
value larger than the statically sized array (8 elements), could this result in
an out-of-bounds access to the ss_status array in kernel memory?

[Severity: Medium]
Is there a potential data race here in etm4_enable_hw()? This patch moves
ss_status to the global drvdata structure, making it shared state. It appears
this modifies the array without acquiring drvdata->spinlock, which could race
with concurrent sysfs operations like sshot_ctrl_store() modifying the same
array.

[ ... ]
> @@ -1081,7 +1081,7 @@ static void etm4_disable_hw(struct etmv4_drvdata *drvdata)
>  
>  	/* read the status of the single shot comparators */
>  	for (i = 0; i < caps->nr_ss_cmp; i++) {
> -		config->ss_status[i] =
> +		drvdata->ss_status[i] =
>  			etm4x_relaxed_read32(csa, TRCSSCSRn(i));

[Severity: Medium]
Does this overwrite the shared ss_status array from hardware in
etm4_disable_hw() without acquiring drvdata->spinlock? A concurrent sysfs
write could clobber these updates.

[Severity: High]
This isn't a bug introduced by this patch, but does this read a trace register
at offset 0x2A0 after power removal has been requested? In etm4_disable_hw(),
the trace unit power-up request (TRCPDCR_PU) is cleared prior to this loop.

According to the architecture specification, software must not access any trace
registers outside the management registers after TRCPDCR.PU is cleared, which
could lead to a system hang or unpredictable behavior.

[ ... ]
> @@ -1511,8 +1511,9 @@ static void etm4_init_arch_data(void *info)
>  	 */
>  	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));
> +		drvdata->ss_status[i] = etm4x_relaxed_read32(csa, TRCSSCSRn(i));

[Severity: High]
This is a pre-existing issue, but does this initialization loop in
etm4_init_arch_data() blindly rely on the uncapped hardware count
caps->nr_ss_cmp? Because drvdata->ss_status is a statically sized 8-element
array, could an unusually large hardware value cause an out-of-bounds write
here?

[ ... ]
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> index 2d8a8f64a0386..ac290f446c51e 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
[ ... ]
> @@ -1829,8 +1829,8 @@ static ssize_t sshot_ctrl_store(struct device *dev,
>  	raw_spin_lock(&drvdata->spinlock);
>  	idx = config->ss_idx;
>  	config->ss_ctrl[idx] = FIELD_PREP(TRCSSCCRn_SAC_ARC_RST_MASK, val);
> -	/* must clear bit 31 in related status register on programming */
> -	config->ss_status[idx] &= ~TRCSSCSRn_STATUS;
> +	/* must clear bit 31 and 30 in related status register on programming */
> +	drvdata->ss_status[idx] &= ~(TRCSSCSRn_STATUS | TRCSSCSRn_PENDING);

[Severity: High]
This isn't a bug introduced by this patch, but does this write in
sshot_ctrl_store() use a potentially out-of-bounds idx to access the
undersized ss_ctrl and ss_status arrays?

Since user input for the index in sshot_idx_store() is bounded to the hardware
capability count rather than the software array boundary, could an attacker
use sysfs to trigger an out-of-bounds write resulting in kernel memory
corruption?

[Severity: Medium]
While the spinlock is held here in sshot_ctrl_store(), does it adequately
protect drvdata->ss_status if concurrent trace sessions in etm4_enable_hw()
and etm4_disable_hw() modify this shared status array without acquiring the
lock?

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