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

[Yeoreum Yun]

On Sat, Oct 03, 2026 at 01:33:45AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] coresight: TOCTOU race in cntr_val_store() defeats active session check
> --
> 
> commit d42ded85664acd4e853cec4a07df95780d0a17bb
> Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> 
> coresight: etm4x: prohibit modifying cntr_val while session is enabled
> 
> This commit adds a check to cntr_val_store() to prevent modifying the
> counter value while the coresight session is active. This prevents user
> configurations from being discarded when the session is disabled.
> 
> > 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]
> Is there a race condition here between the mode check and acquiring the
> spinlock?
> 
> If a user-space thread writes to the cntr_val sysfs file while another
> thread concurrently starts the trace session, cntr_val_store() evaluates
> coresight_get_mode() without holding drvdata->spinlock.
> 
> If the trace session is enabled immediately after this check, the hardware
> is programmed with the old configuration. The sysfs thread then acquires
> the lock and writes the new configuration. When the session is later
> disabled, the old hardware state overwrites the configuration array
> (config->cntr_val), silently losing the user's update.
> 
> Could the mode check be performed inside the drvdata->spinlock critical
> section to prevent this sequence?
> 
> >  	idx = config->cntr_idx;
> >  	config->cntr_val[idx] = val;

No, as another thread I said, This will be following patch by separating
the config and it wouldn't be expected user enabling before finishing
the configuraiton. so ignorable.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261002-separate_etm_cfg_v2-v13-0-fe429d8d96fb@xxxxxxx?part=2

-- 
Sincerely,
Yeoreum Yun