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

[Yeoreum Yun]

On Thu, Sep 24, 2026 at 05:58:14PM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] coresight: TOCTOU race bypasses session-enabled check in sysfs stores
> --
> 
> commit 0c7e96565d9330c7133fb31d646be9050f666c48
> Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> 
> coresight: etm3x: prohibit modifying cntr_val and reset while session is enabled
> 
> This commit prevents cntr_val and reset from being modified while a
> Coresight ETM session is enabled. Changes to cntr_val would be lost when
> the session is disabled, and reset should not be invoked during an active
> session.
> 
> > 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;
> > +
> 
> [Severity: Medium]
> Is there a race condition here since the mode check happens outside the
> spinlock?
> 
> If thread A executes this check and sees the mode is disabled, thread B
> could then enable tracing (acquiring drvdata->spinlock, applying config to
> hardware, and updating the mode). 
> 
> When thread A proceeds, it will acquire the spinlock and zero the
> configuration while the hardware is actively using it. When thread B later
> disables tracing, it would partially overwrite the zeroed configuration with
> hardware register values.
> 
> Should the coresight_get_mode() check be moved inside the spinlock to
> prevent this time-of-check to time-of-use race?

TBH, reset interface seems awful and the same problem already pre-exists.
But this kinda was relief we don't think user wouldn't try to enable
before finishing the reset and return its result.

So this is ignorable.

> 
> >  	if (val) {
> >  		spin_lock(&drvdata->spinlock);
> >  		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;
> 
> [Severity: Medium]
> Does this have the same vulnerability as reset_store()?
> 
> The configuration could be corrupted if tracing is enabled by another thread
> immediately after coresight_get_mode() returns but before the spinlock is
> acquired.
> 
> >  
> >  	spin_lock(&drvdata->spinlock);
> >  	config->cntr_val[config->cntr_idx] = val;

Same above and others

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260924-separate_etm_cfg_v2-v12-0-a2b41d2f7c2b@xxxxxxx?part=3

-- 
Sincerely,
Yeoreum Yun