Re: [PATCH v6 05/14] gtrace: Add functions to start/stop tracing on a component path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gtrace: UAF in comp_start/stop due to missing module/device locks
--

commit ed6a9060977521b7cc68908d59a7104ec2d2492f
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

gtrace: Add functions to start/stop tracing on a component path

The perf driver framework needs to be able to start / stop all components
in a trace component path during its operation. Add gtrace_path_start()
and gtrace_path_stop() functions for this purpose.

> diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
> index f750025104ec9..d7f1df90aa31d 100644
> --- a/drivers/hwtracing/gtrace/gtrace-core.c
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c

[ ... ]

> @@ -611,6 +621,100 @@ static void gtrace_release_path_nodes(struct gtrace_path *path)
>  	}
>  }
>  
> +static int __gtrace_comp_start(struct gtrace_component *comp, pid_t owner)
> +{
> +	struct gtrace_comp_priv *cpriv = to_gtrace_comp_priv(comp);
> +	const struct gtrace_driver *gtdrv = to_gtrace_driver(comp->dev.driver);
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (!gtdrv)
> +		return -ENODEV;
> +
> +	raw_spin_lock_irqsave(&cpriv->lock, flags);

[Severity: High]
Does this lockless driver check create a race with device unbind?

If a privileged user writes to a component's sysfs unbind file or unloads
its module while perf tracing is active, the driver core calls gtrace_remove()
and sets comp->dev.driver to NULL. 

If __gtrace_comp_start() is concurrently called from perf_event_start() in
IRQ context, it could fetch the driver pointer here locklessly just before
it is cleared.

> +	if (cpriv->start_count) {
> +		if (cpriv->owner != owner) {
> +			ret = -EBUSY;
> +			goto out;
> +		}
> +	} else {
> +		if (gtdrv->start) {
> +			ret = gtdrv->start(comp);

[Severity: High]
Could this result in a use-after-free or a panic?

If the race described above occurs, the cached gtdrv pointer might now
point to a freed structure or unmapped driver text, causing a panic when
this callback is invoked inside the lock.

> +			if (ret)
> +				goto out;
> +		}
> +		cpriv->owner = owner;
> +	}
> +
> +	cpriv->start_count++;
> +out:
> +	raw_spin_unlock_irqrestore(&cpriv->lock, flags);
> +	return ret;
> +}
> +
> +static int __gtrace_comp_stop(struct gtrace_component *comp)
> +{
> +	struct gtrace_comp_priv *cpriv = to_gtrace_comp_priv(comp);
> +	const struct gtrace_driver *gtdrv = to_gtrace_driver(comp->dev.driver);
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	raw_spin_lock_irqsave(&cpriv->lock, flags);

[Severity: High]
Does this have the same race as __gtrace_comp_start()?

The driver pointer is cached locklessly here as well, exposing the stop
callback to a race with driver removal.

> +	if (!cpriv->start_count) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	cpriv->start_count--;
> +	if (!cpriv->start_count) {
> +		if (!gtdrv) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +
> +		if (gtdrv->stop)
> +			ret = gtdrv->stop(comp);

[Severity: High]
Could this trigger a use-after-free during the stop path?

If the driver has been removed and the structure freed, calling this
potentially freed stop callback could crash the kernel.

> +	}
> +out:
> +	raw_spin_unlock_irqrestore(&cpriv->lock, flags);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001052243.3042627-1-mayuresh.chitale@oss.qualcomm.com?part=5