Re: [PATCH v6 07/14] gtrace: Add function to copy into perf AUX buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gtrace: wild pointer and UAF during concurrent driver unbind
- [Medium] gtrace: lock mismatch by iterating over path in gtrace_path_copyto_auxbuf()
--

commit 727fcf662499fff522c570fa7a02e693a5593303
Author: Anup Patel <anup.patel@oss.qualcomm.com>

gtrace: Add function to copy into perf AUX buffer

The RISC-V trace ramsink will need a mechanism to copy trace data
into the perf AUX buffer. Add gtrace_path_copyto_auxbuf() function
and corresponding trace driver callback copyto_auxbuf() for this
purpose.

> diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
> index d7f1df90aa31d..acbeb3df21354 100644
> --- a/drivers/hwtracing/gtrace/gtrace-core.c
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c
> @@ -715,6 +715,47 @@ int gtrace_path_stop(struct gtrace_path *path)
>  }
>  EXPORT_SYMBOL_GPL(gtrace_path_stop);
>  
> +int gtrace_path_copyto_auxbuf(struct gtrace_path *path,
> +			      struct gtrace_perf_auxbuf *buf,
> +			      size_t *bytes_copied, u64 *format)
> +{
> +	struct gtrace_comp_priv *sink = to_gtrace_comp_priv(gtrace_path_sink(path));
> +	const struct gtrace_driver *gtdrv;
> +	struct gtrace_component *comp;
> +	struct gtrace_path_node *node;
> +	int ret = -EOPNOTSUPP;
> +	unsigned long flags;
> +
> +	/*
> +	 * Copy only after every path using the sink has stopped; otherwise the
> +	 * hardware may overwrite data being copied, or the same data may be copied
> +	 * to aux buffer twice. The last path to stop does the copy. Holding the
> +	 * sink's lock also stops another path from starting the sink before copy
> +	 * completes.
> +	 */
> +	raw_spin_lock_irqsave(&sink->lock, flags);
> +	if (sink->start_count) {
> +		*bytes_copied = 0;
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	list_for_each_entry(node, &path->comp_list, head) {

[Severity: Medium]
Will this loop cause a lock mismatch if an intermediate component implements
copyto_auxbuf?

The function acquires sink->lock, but iterates over path->comp_list to find
and execute the callback. If a non-sink component is matched, its callback
will run under the sink's lock rather than its own, which may allow data races
with concurrent start or stop operations on that intermediate component.

> +		comp = node->comp;
> +		gtdrv = to_gtrace_driver(comp->dev.driver);

[Severity: High]
Does this code allow concurrent driver unbind to cause a use-after-free or
NULL pointer offset dereference?

The to_gtrace_driver() macro double-evaluates comp->dev.driver. If it is set
to NULL by a concurrent unbind between evaluations, the macro yields a negative
offset rather than NULL. This bypasses the !gtdrv check and dereferences an
invalid pointer.

Additionally, since no device lock or module reference is held here, can the
driver be unloaded while the callback is executing?

> +		if (!gtdrv || !gtdrv->copyto_auxbuf)
> +			continue;
> +
> +		*bytes_copied = gtdrv->copyto_auxbuf(comp, buf, format);
> +		ret = 0;
> +		break;
> +	}
> +out:
> +	raw_spin_unlock_irqrestore(&sink->lock, flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gtrace_path_copyto_auxbuf);

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