Re: [PATCH RESEND v13 02/14] coresight: etm4x: prohibit modifying cntr_val while session is enabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] coresight: TOCTOU race in cntr_val_store due to lockless mode check
--

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

coresight: etm4x: prohibit modifying cntr_val while session is enabled

This commit adds a check in cntr_val_store() to prevent modifying the
cntr_val configuration while a Coresight session is enabled. It returns
-EBUSY if the session is currently active.

> 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
> @@ -1615,6 +1615,9 @@ static ssize_t cntr_val_store(struct device *dev,
>  		return -EINVAL;
>  	if (val > ETM_CNTR_MAX_VAL)
>  		return -EINVAL;
> +	if (IS_ERR_OR_NULL(drvdata->csdev) ||
> +	    coresight_get_mode(drvdata->csdev))
> +		return -EBUSY;
>  
>  	raw_spin_lock(&drvdata->spinlock);

[Severity: Medium]
Could this leave a race window before the state is modified?

Since the mode check is performed locklessly before acquiring
drvdata->spinlock, another process could enable the session concurrently. If
the session is enabled right after this check evaluates to false, but before
cntr_val_store() acquires the lock, the configuration update would be lost.

The concurrent enablement would set the mode and call etm4_enable_sysfs()
to program the hardware. If cntr_val_store() acquires the lock after this,
it updates the software state which is never flushed to hardware for the
active session, and will eventually be overwritten by the old hardware state
during session teardown.

Should the mode check be moved inside the spinlock critical section to
properly serialize with session enablement?

>  	idx = config->cntr_idx;
>  	config->cntr_val[idx] = val;
>  	raw_spin_unlock(&drvdata->spinlock);
>  	return size;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-separate_etm_cfg_v2-v13-0-5d8a29aead95@arm.com?part=2