Re: [PATCH v12 01/14] coresight: etm4x: read-back TRCSEQSTR at disabling and prohibit modifying seq_state while enabling.

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] coresight: TOCTOU race in seq_state_store lockless mode check
> - [Low] coresight: misplaced IS_ERR_OR_NULL(drvdata) after dereference
> --
> 
> commit fb240199cfa6db080873efbe2c007beb70c01070
> Author: Yeoreum Yun <yeoreum.yun@arm.com>
> 
> coresight: etm4x: read-back TRCSEQSTR at disabling and prohibit modifying seq_state while enabling.
> 
> TRCSEQSTR can be changed by the ETM while a session is active. Its value
> therefore needs to be preserved when a sysfs session is disabled so that
> users can read the current sequence state after the session ends.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > index cc6cdd3ae29d5..db3cf4ad6806c 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > @@ -1437,6 +1437,8 @@ static ssize_t seq_state_store(struct device *dev,
> >  		return -EINVAL;
> >  	if (val >= drvdata->nrseqstate)
> >  		return -EINVAL;
> > +	if (IS_ERR_OR_NULL(drvdata) || coresight_get_mode(drvdata->csdev))
> > +		return -EBUSY;
> 
> [Severity: Low]
> Does this code introduce a regression by checking drvdata for NULL after it
> has already been dereferenced?
> 
> Looking at seq_state_store(), drvdata is dereferenced during the check for
> val >= drvdata->nrseqstate before IS_ERR_OR_NULL(drvdata) is called.

Yes. I'll fix it.

> 
> While drvdata is fundamentally guaranteed to be valid in this sysfs context,
> making the check technically dead code rather than a reachable crash, should
> the validation be moved before the dereference if it is kept?
> 
> >  
> >  	config->seq_state = val;
> 
> [Severity: Medium]
> Could this lockless mode check cause a regression?
> 
> A concurrent thread could enable the trace session immediately after the
> lockless coresight_get_mode() check evaluates to disabled. This would allow
> config->seq_state to be modified while the session is actively starting or
> already active, completely defeating the patch's stated goal of prohibiting
> state modification during an active session.
> 
> Should raw_spin_lock(&drvdata->spinlock) be held across both the mode check
> and the subsequent state update to prevent this regression?
> 
> >  	return size;
> >  }

This doesn't matter, TBH I don't expect user concurrently setup the
configuration while session is not ended yet and similar problem
is already exist before separation of this patch too.

So, this is ignorable.

-- 
Sincerely,
Yeoreum Yun