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

[sashiko-bot]

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

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

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

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

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

>  	idx = config->ss_idx;

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