Re: [PATCH v5 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 6 potential issue(s) to consider:
- [High] Asymmetric trace ID lifecycle causes `put_trace_id` to be called on components that did not allocate an ID.
- [High] `gtrace_create_path` returns a path object with an empty component list when the sink is unreachable, leading to out-of-bounds pointer dereferences.
- [High] Missing NULL checks on component driver pointers (`gtdrv`) leads to NULL pointer dereferences.
- [High] Recursive graph traversal crashes on unresolved output connections where the destination component is NULL.
- [High] Unbounded recursive depth-first search (DFS) graph traversal lacks cycle detection, leading to kernel stack overflow.
- [High] Lockless pointer return in `gtrace_cpu_source` leads to a Use-After-Free.
--

--- Patch [4]: [PATCH v5 04/14] gtrace: Add functions to create/destroy a trace component path ---
commit a8e1889600bd5a16b5c7f9f06b462a376b8f943f
Author: Anup Patel <anup.patel@oss.qualcomm.com>

gtrace: Add functions to create/destroy a trace component path
    
Trace needs to be configured on a chain of trace components which are
connected to each other. These chain of components is also referred
to as trace component path. Add functions to create/destroy a trace
component path which will be later used by gtrace perf support.

> diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
> --- a/drivers/hwtracing/gtrace/gtrace-core.c
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c
> @@ -210,6 +210,53 @@ int gtrace_reset_component(struct gtrace_component *comp)
>  }
>  EXPORT_SYMBOL_GPL(gtrace_reset_component);
>  
> +static int __gtrace_walk_output_components(struct gtrace_component *comp,
> +					   bool *stop, void *priv,
> +					   int (*fn)(struct gtrace_component *comp, bool *stop,
> +						     struct gtrace_connection *stop_conn,
> +						     void *priv))
> +{
> +	struct gtrace_connection *conn, *stop_conn = NULL;
> +	struct gtrace_platform_data *pdata = comp->pdata;

[Severity: High]
Does this crash if comp is NULL?

If a destination component is missing or unregistered, conn->dest_comp will be
NULL. When the recursive call is made, comp->pdata is evaluated immediately
without verifying comp, which would cause a NULL pointer dereference.

> +	int i, ret;
> +
> +	for (i = 0; i < pdata->nr_outconns; i++) {
> +		conn = pdata->outconns[i];
> +		ret = __gtrace_walk_output_components(conn->dest_comp, stop, priv, fn);

[Severity: High]
Is there any cycle detection in this graph traversal?

If a misconfigured Device Tree provides a cyclical graph layout
(e.g., A -> B -> A), wouldn't this DFS recurse indefinitely and exhaust the
kernel stack?

[ ... ]

>  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]
Is it safe to return this pointer without taking a reference?

If a caller obtains the component pointer while gtrace_unregister_component()
is invoked concurrently, the entry could be cleared and the component freed.
This would leave the caller with a dangling pointer.

Should this use gtrace_get_component() or RCU read locks before returning?

[ ... ]

> +struct gtrace_component *gtrace_path_source(struct gtrace_path *path)
> +{
> +	struct gtrace_path_node *node;
> +
> +	node = list_first_entry(&path->comp_list, struct gtrace_path_node, head);
> +	return node->comp;
> +}
> +EXPORT_SYMBOL_GPL(gtrace_path_source);

[Severity: High]
Does list_first_entry() resolve to an invalid offset if the list is empty?

If gtrace_create_path() successfully returns a path with an empty list,
calling gtrace_path_source() maps the pointer over the list head itself,
causing node->comp to read garbage memory.

[ ... ]

> +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->get_trace_id)

[Severity: High]
What happens if the component device lacks a bound driver?

The to_gtrace_driver() macro handles NULL inputs by returning NULL. If gtdrv
is NULL, evaluating !gtdrv->get_trace_id will dereference a NULL pointer.

> +			continue;
> +
> +		trace_id = gtdrv->get_trace_id(comp, path->mode);
> +		if (trace_id > 0) {
> +			path->trace_id = trace_id;
> +			return 0;

[Severity: High]
This function stops and returns at the first component that successfully
allocates a trace ID.

[ ... ]

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

[Severity: High]
Since gtrace_assign_trace_id() only allocates an ID from the first capable
component, wouldn't this loop unconditionally call put_trace_id() for every
component in the path?

This could ask components to free a trace ID they never allocated,
potentially corrupting their allocation bitmaps or driver state.

[ ... ]

> +struct gtrace_path *gtrace_create_path(struct gtrace_component *source,
> +				       struct gtrace_component *sink,
> +				       enum gtrace_component_mode mode)
> +{
> +	struct build_path_walk_priv priv;
> +	struct gtrace_path *path;
> +	int ret = 0;
> +
> +	if (!source || mode >= GTRACE_COMPONENT_MODE_MAX) {
> +		ret = -EINVAL;
> +		goto err_out;
> +	}
> +
> +	path = kzalloc(sizeof(*path), GFP_KERNEL);
> +	if (!path) {
> +		ret = -ENOMEM;
> +		goto err_out;
> +	}
> +	INIT_LIST_HEAD(&path->comp_list);
> +	path->mode = mode;
> +	path->trace_id = GTRACE_INVALID_TRACE_ID;
> +
> +	priv.path = path;
> +	priv.sink = sink;
> +	ret = gtrace_walk_output_components(source, &priv, build_path_walk_fn);
> +	if (ret < 0)
> +		goto err_release_path_nodes;

[Severity: High]
If the destination sink is unreachable, gtrace_walk_output_components()
completes without adding any nodes to path->comp_list and returns 0.

Since gtrace_path_ready() and gtrace_assign_trace_id() will vacuously pass
for an empty list, won't this return a successful path object with an empty
component list?

This leads to the invalid list head dereferences mentioned earlier in
gtrace_path_source() and gtrace_path_sink().

> +
> +	if (!gtrace_path_ready(path)) {
> +		ret = -EOPNOTSUPP;
> +		goto err_release_path_nodes;
> +	}

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