Re: [PATCH v13 03/14] coresight: etm3x: prohibit modifying cntr_val and reset while session is enabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] coresight: TOCTOU race in sysfs configuration modifications
--

commit 58a68349c3bcb900219eaabae6561553d37341df
Author: Yeoreum Yun <yeoreum.yun@arm.com>

coresight: etm3x: prohibit modifying cntr_val and reset while session is enabled

This commit prevents modifying cntr_val and resetting the state when the
trace session is enabled. It does so by checking the coresight mode before
proceeding with modifications in sysfs store functions.

> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> index b3c67e96a82a4..cdb70a4382eb8 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> @@ -75,6 +75,10 @@ static ssize_t reset_store(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> +	if (IS_ERR_OR_NULL(drvdata->csdev) ||
> +	    coresight_get_mode(drvdata->csdev))
> +		return -EBUSY;
> +
>  	if (val) {
>  		spin_lock(&drvdata->spinlock);

[Severity: High]
Does this code introduce a TOCTOU race condition?

The coresight_get_mode() check is evaluated outside the drvdata->spinlock
that serializes hardware and state changes.

If a concurrent thread writes to the sysfs reset attribute while another
thread enables the trace session via enable_source, couldn't the sysfs store
function's check pass just before the session enablement code takes the
mode (via coresight_take_mode()) without holding the spinlock?

The sysfs store would then acquire the spinlock, clear the config, and
release the trace ID while the session is actively building its path and
starting up.

This could corrupt the trace configuration, allow a trace session to start
with a zeroed-out configuration, and prematurely release the actively
assigned trace ID, causing trace ID conflicts.

This issue was verified to remain unfixed in the final state of the patch
series (commit 45def19b707827632be083676fa86077f5bc1585).

>  		memset(config, 0, sizeof(struct etm_config));

[ ... ]

> @@ -744,6 +748,9 @@ static ssize_t cntr_val_store(struct device *dev,
>  	ret = kstrtoul(buf, 16, &val);
>  	if (ret)
>  		return ret;
> +	if (IS_ERR_OR_NULL(drvdata->csdev) ||
> +	    coresight_get_mode(drvdata->csdev))
> +		return -EBUSY;
>  
>  	spin_lock(&drvdata->spinlock);

[Severity: High]
Similarly, does this safety check fail to guard concurrent updates
to cntr_val?

The check is performed outside the spinlock, creating the same race window
where cntr_val could be updated while the session is being enabled.

>  	config->cntr_val[config->cntr_idx] = val;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-separate_etm_cfg_v2-v13-0-fe429d8d96fb@arm.com?part=3