Re: [PATCH v3 2/2] interconnect: qcom: add MSM8x60 NoC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Memory leak of dynamically allocated target nodes on probe failure.
- [High] Use-After-Free (UAF) of cross-fabric nodes upon driver unbind.
- [High] Bandwidth starvation due to incorrect node clock rate calculation.
--

commit 5d22619e7f59ed3d33c0640a610abb2e710a6a9c
Author: Herman van Hazendonk <github.com@herrie.org>

interconnect: qcom: add MSM8x60 NoC driver

Add a Qualcomm interconnect driver for the MSM8x60 family modelling the
four NoC fabrics (APPSS, System, MMSS, Daytona) that connect masters
and slaves on these Scorpion-class SoCs.

> diff --git a/drivers/interconnect/qcom/msm8660.c b/drivers/interconnect/qcom/msm8660.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/interconnect/qcom/msm8660.c
[ ... ]
> @@ -1231,23 +1231,23 @@ static int msm8660_icc_set(struct icc_node *src, struct icc_node *dst)
[ ... ]
> +	list_for_each_entry(n, &provider->nodes, node_list) {
> +		u64 node_bw = max(icc_units_to_bps(n->avg_bw),
> +				  icc_units_to_bps(n->peak_bw));
> +		u64 node_rate = div_u64(node_bw, qp->desc->bus_width);

[Severity: High]
Does this calculation incorrectly starve narrow links by using the global
fabric bus width?

The clock rate is calculated by dividing the node's bandwidth by the overall
fabric bus width (qp->desc->bus_width). However, some peripheral nodes
(like sfab_to_system_fpb) have a narrower bus width (4 bytes) than the full
fabric (8 bytes).

Dividing a 4-byte node's bandwidth by 8 instead of its own width halves the
resulting requested clock frequency, mathematically guaranteeing these narrower
links will be starved of bandwidth.

Should this use ((struct msm8660_icc_node *)n->data)->buswidth instead?

[ ... ]
> @@ -1563,30 +1563,30 @@ static int msm8660_icc_probe(struct platform_device *pdev)
[ ... ]
> +		for (j = 0; j < qnodes[i]->num_links; j++) {
> +			ret = icc_link_nodes(node,
> +					     &qnodes[i]->link_nodes[j]->node);
> +			if (ret)
> +				goto err_remove_nodes;
> +		}
> +
> +		data->nodes[i] = node;
> +	}
> +
> +	ret = icc_provider_register(provider);
> +	if (ret)
> +		goto err_remove_nodes;
> +
> +	platform_set_drvdata(pdev, qp);
> +
> +	dev_info(dev, "MSM8660 interconnect provider registered\n");
> +
> +	return 0;
> +
> +err_remove_nodes:
> +	icc_nodes_remove(provider);
> +	msm8660_clear_node_cache(qnodes, num_nodes);

[Severity: High]
Does this error path leak memory for target nodes that were allocated
during icc_link_nodes() but not yet added to the provider?

If probe fails (e.g., -ENOMEM in icc_node_set_name()), the error path calls
icc_nodes_remove(), which frees nodes that have already been fully initialized
and added to provider->nodes.

The driver then calls msm8660_clear_node_cache(), which zeroes all
qnodes[k]->node pointers. This destroys the only reference to any un-added
target nodes that were allocated mid-loop by icc_link_nodes(), resulting in a
memory leak.

[ ... ]
> @@ -1604,11 +1604,11 @@ static void msm8660_icc_remove(struct platform_device *pdev)
> +static void msm8660_icc_remove(struct platform_device *pdev)
> +{
> +	struct msm8660_icc_provider *qp = platform_get_drvdata(pdev);
> +	const struct msm8660_icc_desc *desc = of_device_get_match_data(&pdev->dev);
> +
> +	icc_provider_deregister(&qp->provider);
> +	icc_nodes_remove(&qp->provider);

[Severity: High]
Can this cause a use-after-free of cross-fabric nodes if a fabric is unbound
via sysfs?

The interconnect fabrics are registered as separate platform devices.
Cross-fabric links are established by storing a raw pointer to the target
node in the source node's links array.

Because there is no .suppress_bind_attrs = true, fabrics can be individually
unbound via sysfs. Unbinding a target fabric triggers icc_nodes_remove(),
which destroys and frees its nodes. The still-bound source fabric then retains
a dangling pointer to the freed node in its links array, which will be
dereferenced during path finding operations.

> +	if (desc)
> +		msm8660_clear_node_cache(desc->nodes, desc->num_nodes);
> +	/* clk cleanup happens via devm_add_action_or_reset on remove. */
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606-submit-interconnect-msm8660-v3-0-f9da0158cdf8@herrie.org?part=2