Re: [PATCH v6 04/14] gtrace: Add functions to create/destroy a trace component path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gtrace: lockless driver callbacks lead to unmapped memory execution
- [High] gtrace: gtrace_cpu_source returns component without holding reference
--

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

gtrace: Add functions to create/destroy a trace component path

This patch adds functions to create and destroy a trace component path.
A trace component path represents a chain of trace components connected
to each other, which will be used by gtrace perf support.

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

[ ... ]

>  struct gtrace_component *gtrace_cpu_source(unsigned int cpu)
>  {
>  	if (!cpu_present(cpu))
>  		return NULL;
>  
>  	return per_cpu(gtrace_cpu_source_comp, cpu);
>  }

[Severity: High]
Does this return the component without holding a reference to it?

If a caller operates on this pointer without pinning it, and the component is
concurrently unregistered via gtrace_unregister_component(), could this lead
to a use-after-free? When the caller later passes this pointer to
gtrace_create_path(), it would be dereferenced during the walk of output
components.

Should this hold gtrace_mutex or take a reference via get_device() before
returning the component?

[ ... ]

> +static int gtrace_assign_trace_id(struct gtrace_path *path)
> +{
> +	const struct gtrace_driver *gtdrv;
> +	struct gtrace_component *comp;
> +	struct gtrace_path_node *node;
> +	int trace_id;
> +
> +	list_for_each_entry(node, &path->comp_list, head) {
> +		comp = node->comp;
> +		gtdrv = to_gtrace_driver(comp->dev.driver);
> +
> +		if (!gtdrv || !gtdrv->get_trace_id)
> +			continue;
> +
> +		trace_id = gtdrv->get_trace_id(comp, path->mode);

[Severity: High]
Can this locklessly access the driver and execute unmapped memory?

Taking a device reference during path creation prevents the device memory from
being freed, but does it prevent the module from unloading? If the component
module is unloaded concurrently, comp->dev.driver is cleared and the module
code is freed.

If a racing thread fetches the driver pointer just prior to unbind, could it
subsequently invoke gtdrv->get_trace_id() after the function has been unmapped,
resulting in a kernel panic?

> +		if (trace_id > 0) {
> +			path->trace_id = trace_id;
> +			return 0;
> +		} else if (trace_id < 0) {

[ ... ]

> +static bool gtrace_path_ready(struct gtrace_path *path)
> +{
> +	struct gtrace_comp_priv *cpriv;
> +	struct gtrace_path_node *node;
> +
> +	list_for_each_entry(node, &path->comp_list, head) {
> +		cpriv = to_gtrace_comp_priv(node->comp);
> +		if (!cpriv->ready)
> +			return false;

[Severity: High]
Is it safe to read cpriv->ready here without holding the device_lock?

If the driver is concurrently unbound, could this be reading a value that is
no longer protected against modification?

> +	}
> +
> +	return true;
> +}

[ ... ]

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