Re: [PATCH v5 03/14] gtrace: Add RISC-V platform driver for the gtrace framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:

New issues:
- [High] Missing `IS_ERR` check for `gtrace_find_by_fwnode()` return value.
- [High] The CPU device node is used in a format string after its reference count has been decremented.
- [High] Dropping fwnode references before unregistering the component causes a use-after-free in the core subsystem.
- [Medium] Device node references `rep_node` and `rdev_node` are leaked on the success path in `rvtrace_of_parse_outconns()`.
- [Medium] Leak of destination fwnode reference when component lookup fails in `rvtrace_of_parse_outconns()`.
- [Medium] Output connection fwnode references are leaked if `rvtrace_platform_probe()` fails after parsing connections.
- [Medium] The driver lacks a `MODULE_DEVICE_TABLE` declaration for its OF match table, preventing automatic module loading.
- [Medium] Unprotected read-modify-write operations on hardware control registers.

Pre-existing issues:
- [High] `gtrace_find_by_fwnode()` returns a component pointer without acquiring a reference.
--

--- Patch [3]: [PATCH v5 03/14] gtrace: Add RISC-V platform driver for the gtrace framework ---
commit d15655615f1b10f39198151874d1d481ed52c740
Author: Anup Patel <anup.patel@oss.qualcomm.com>

gtrace: Add RISC-V platform driver for the gtrace framework

The RISC-V trace components defined by the RISC-V Trace Control Interface
Specification are memory mapped devices which are organized in a
graph-like topology and described in the device tree.

Add the RISC-V platform driver for the gtrace framework. The driver
parses the RISC-V component topology from the device tree, discovers the
type and version of each component from its IMPL register, provides the
RISC-V register level control ops and registers each component with the
gtrace core.

Co-developed-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>

> diff --git a/drivers/hwtracing/gtrace/rvtrace-platform.c b/drivers/hwtracing/gtrace/rvtrace-platform.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/rvtrace-platform.c
> @@ -0,0 +1,286 @@
[ ... ]
> +static int rvtrace_hw_enable(struct gtrace_platform_data *pdata)
> +{
> +	u32 val;
> +
> +	val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	val |= BIT(RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT);
> +	gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);

[Severity: Medium]
Are these read-modify-write operations safe from concurrent access?

Looking at rvtrace_hw_enable() and rvtrace_hw_disable(), the sequences 
gtrace_read32() followed by gtrace_write32() execute locklessly. 

If the tracing framework or userspace allows concurrent control, could this 
corrupt the RVTRACE_COMPONENT_CTRL_OFFSET register state?

> +	return gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
> +			       RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT, 1,
> +			       pdata->control_poll_timeout_usecs);
> +}
> +
> +static int rvtrace_hw_disable(struct gtrace_platform_data *pdata)
> +{
> +	u32 val;
> +
> +	val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	val &= ~BIT(RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT);
> +	gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	return gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
> +			       RVTRACE_COMPONENT_CTRL_ENABLE_SHIFT, 0,
> +			       pdata->control_poll_timeout_usecs);
> +}
[ ... ]
> +static int rvtrace_of_parse_outconns(struct gtrace_platform_data *pdata)
> +{
[ ... ]
> +		conn->dest_comp = gtrace_find_by_fwnode(conn->dest_fwnode);
> +		if (!conn->dest_comp) {

[Severity: High]
Does this check properly handle error pointers?

gtrace_find_by_fwnode() returns an ERR_PTR on failure. Since the check is
only for NULL, will an error pointer bypass this condition and be
assigned to conn->dest_comp as a valid pointer, potentially causing a
kernel panic later when dereferenced?

[Severity: High]
This isn't a bug introduced by this patch, but does gtrace_find_by_fwnode()
return a component pointer without acquiring a reference to it?

If we look at gtrace_match_fwnode() in the core subsystem:

drivers/hwtracing/gtrace/gtrace-core.c:gtrace_match_fwnode() {
	if (device_match_fwnode(&comp->dev, d->fwnode)) {
		d->match = comp;
		return 1;
	}
}

The component pointer is returned without incrementing its reference count.

Could this lead to a use-after-free if the matched component is
concurrently unregistered before we use it here?

[Severity: Medium]
Does this error path leak the dest_fwnode reference?

We acquired a reference to conn->dest_fwnode earlier with
fwnode_handle_get(). If gtrace_find_by_fwnode() fails and we break out
of the loop here, conn is not yet assigned to pdata->outconns.

The cleanup block at the end only frees references that were already
assigned to the pdata->outconns array.

Will this unassigned conn's fwnode reference be permanently leaked?

> +			ret = -EPROBE_DEFER;
> +			of_node_put(ep_node);
> +			of_node_put(rep_node);
> +			of_node_put(rdev_node);
> +			break;
> +		}
> +
> +		pdata->outconns[i] = conn;
> +		i++;
> +	}

[Severity: Medium]
Are the device node references for rep_node and rdev_node leaked on the
success path?

of_graph_get_remote_endpoint() and of_graph_get_port_parent() return
device nodes with incremented reference counts. 

When the loop continues to the next iteration after a successful parse, 
it doesn't appear that of_node_put() is called for these nodes.

[ ... ]
> +static int rvtrace_platform_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	node = of_parse_phandle(dev_of_node(dev), "cpus", 0);
> +	if (node) {
> +		ret = of_cpu_node_to_id(node);
> +		of_node_put(node);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "failed to get CPU id for %pOF\n", node);

[Severity: High]
Does this error path trigger a use-after-free on the device node?

of_node_put(node) is called to drop the reference to the CPU device node.
Immediately afterward on the error path, the same node pointer is passed
to dev_err_probe() using the %pOF format specifier.

Could the format parsing dereference the freed node memory?

> +		pdata->bound_cpu = ret;
> +	}
> +
> +	/* Default control poll timeout */
> +	pdata->control_poll_timeout_usecs = 10;
> +
> +	ret = rvtrace_of_parse_outconns(pdata);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to parse output connections\n");
> +
> +	ret = rvtrace_of_parse_inconns(pdata);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to parse input connections\n");

[Severity: Medium]
Will the fwnode references acquired in rvtrace_of_parse_outconns() be
leaked if initialization fails here?

If rvtrace_of_parse_inconns() or any subsequent initialization step fails,
rvtrace_platform_probe() returns directly. 

Since there is no cleanup path to iterate through pdata->outconns and call 
fwnode_handle_put(), will all the acquired fwnode references be permanently 
leaked?

[ ... ]
> +static void rvtrace_platform_remove(struct platform_device *pdev)
> +{
> +	struct gtrace_component *comp = platform_get_drvdata(pdev);
> +	struct gtrace_platform_data *pdata = comp->pdata;
> +	struct gtrace_connection *conn;
> +	int i;
> +
> +	for (i = 0; i < pdata->nr_outconns; i++) {
> +		conn = pdata->outconns[i];
> +		if (conn && conn->dest_fwnode)
> +			fwnode_handle_put(conn->dest_fwnode);
> +	}
> +
> +	gtrace_unregister_component(comp);
> +}

[Severity: High]
Could dropping the fwnode references before unregistering the component
cause a use-after-free in the core subsystem?

Here, the driver drops the references to the destination fwnodes before
calling gtrace_unregister_component(). The core unregistration function
iterates over the connections and accesses the fwnodes via
device_match_fwnode().

Since the references were already dropped, could the memory have been freed,
causing a use-after-free during cleanup?

> +static const struct of_device_id rvtrace_platform_match[] = {
> +	{ .compatible = "riscv,trace-component" },
> +	{}
> +};

[Severity: Medium]
Is a MODULE_DEVICE_TABLE declaration missing for this OF match table?

Platform drivers configured as modules typically expose their device ID
tables using MODULE_DEVICE_TABLE(of, rvtrace_platform_match). 

Without it, modaliases won't be exported, which prevents udev from 
automatically loading the driver when matching hardware is present.

[ ... ]

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