Re: [PATCH v11 1/9] coresight: etm4x: prohibit modifying ss_status and cntr_val while session is enabled

[Yeoreum Yun]

On Tue, Sep 15, 2026 at 11:51:15AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] coresight: NULL pointer dereference in sysfs stores during device initialization
> - [Medium] coresight: TOCTOU race in sysfs store functions bypasses session check
> --
> 
> commit e0461b6a1ca35a227d37c4654e32e424dc9d60f7
> Author: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> 
> coresight: etm4x: prohibit modifying ss_status and cntr_val while session is enabled
> 
> This commit adds checks to prohibit modifying ss_status and cntr_val via sysfs
> while a trace session is enabled. It prevents changes to these values from
> being silently overwritten and lost when the session is disabled.
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > index cc6cdd3ae29d5..8d73248b840a6 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > @@ -1613,6 +1613,8 @@ static ssize_t cntr_val_store(struct device *dev,
> >  		return -EINVAL;
> >  	if (val > ETM_CNTR_MAX_VAL)
> >  		return -EINVAL;
> > +	if (coresight_get_mode(drvdata->csdev))
> 
> [Severity: High]
> Can drvdata->csdev be NULL here if this sysfs attribute is accessed during
> device initialization?
> 
> If userspace writes to this sysfs attribute immediately upon device creation
> (e.g., via udev rules), coresight_register() might not have returned yet to
> assign drvdata->csdev in etm4_probe(), leading to a NULL pointer dereference.
> 
> Could we use to_coresight_device(dev) instead to avoid this?
> 
> > +		return -EBUSY;

Vaild. theorically, It's possible but it seems enoguh to check whether
drvdata->csdev is NULL or not.

> >  
> >  	raw_spin_lock(&drvdata->spinlock);
> 
> [Severity: Medium]
> Is there a race condition here since the mode check happens before acquiring
> the spinlock?
> 
> If Thread A writes to cntr_val and passes the mode check, Thread B could
> concurrently enable the session (taking the spinlock, configuring the hardware,
> and changing the mode). Thread A would then acquire the spinlock and modify
> the software configuration, leaving it out of sync with the active hardware.
> 
> Should the mode check be moved inside the spinlock?

Not matter. anyway the etmX_enable() will set with the value when it
see. This is for preventing overwriting at the time of disable.
not the race in the enable. So it is ignorable.

> 
> >  	idx = config->cntr_idx;
> [ ... ]
> > @@ -1797,6 +1799,8 @@ static ssize_t sshot_ctrl_store(struct device *dev,
> >  
> >  	if (kstrtoul(buf, 16, &val))
> >  		return -EINVAL;
> > +	if (coresight_get_mode(drvdata->csdev))
> 
> [Severity: High]
> Does this suffer from the same NULL pointer dereference risk during device
> initialization as cntr_val_store()?
> 
> > +		return -EBUSY;
> >  
> >  	raw_spin_lock(&drvdata->spinlock);

Same above. Will add NULL check.

> 
> [Severity: Medium]
> Is this check also vulnerable to the same race condition by being outside
> the spinlock?

See above.

> 
> >  	idx = config->ss_idx;
> [ ... ]
> > @@ -1847,6 +1851,8 @@ static ssize_t sshot_pe_ctrl_store(struct device *dev,
> >  
> >  	if (kstrtoul(buf, 16, &val))
> >  		return -EINVAL;
> > +	if (coresight_get_mode(drvdata->csdev))
> 
> [Severity: High]
> Does this suffer from the same NULL pointer dereference risk during device
> initialization as cntr_val_store()?
> 
> > +		return -EBUSY;
> >  
> >  	raw_spin_lock(&drvdata->spinlock);

Same above. Will add NULL check.

> 
> [Severity: Medium]
> Is this check also vulnerable to the same race condition by being outside
> the spinlock?
> 
> >  	idx = config->ss_idx;

Ignorable.


> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260915-separate_etm_cfg_v2-v11-0-d2b258d51747@xxxxxxx?part=1

-- 
Sincerely,
Yeoreum Yun