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

[Yeoreum Yun <yeoreum.yun@arm.com>]

> 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));

This is known issue and it's pre-exist one. but we don't expect
user enabling session before *reset* is returned to user.

So this is ignorable.

> 
> > @@ -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;

As I said another thread and the same above. also the the separation
of config will relax this problem.

-- 
Sincerely,
Yeoreum Yun