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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Clock prepare/enable reference leak on probe deferral when RPM device lookup fails.
- [High] Time-of-check to time-of-use (TOCTOU) race condition in `msm8660_get_rpm` leading to Use-After-Free.
- [High] Integer truncation in the fabric clock rate calculation allows unprivileged processes to freeze the interconnect bus.
- [Low] `COUNT_ARGS(__VA_ARGS__)` evaluates to 1 when no arguments are passed, creating bogus links to node ID 0.
--

commit 3b79422423710904c503ef7d19429d69c2f02d18
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 fabrics that connect masters and slaves on these Scorpion-class SoCs.
The driver implements the interconnect-provider API to manage bandwidth
between specific masters and slaves.

> diff --git a/drivers/interconnect/qcom/msm8660.c b/drivers/interconnect/qcom/msm8660.c
> new file mode 100644
> index 0000000000000..174bc59da74f7
> --- /dev/null
> +++ b/drivers/interconnect/qcom/msm8660.c
> @@ -0,0 +1,1069 @@
[ ... ]
> +#define DEFINE_QNODE(_name, _id, _buswidth, _mas, _slv, _tier, ...)	\
> +	static struct msm8660_icc_node _name = {			\
> +		.name = #_name,						\
> +		.id = _id,						\
> +		.buswidth = _buswidth,					\
> +		.num_links = COUNT_ARGS(__VA_ARGS__),			\

[Severity: Low]
Does COUNT_ARGS(__VA_ARGS__) evaluate to 1 when invoked with no links?

When DEFINE_QNODE is invoked with an empty variadic argument list, this
might receive a single empty token and initialize .links to { 0 },
causing a phantom node with ID 0 to be created.

[ ... ]
> +static int msm8660_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
[ ... ]
> +	/* Apply minimum floor to prevent bus starvation */
> +	rate = max_t(u64, rate, MSM8660_FABRIC_MIN_RATE);
> +	rate = min_t(u32, rate, INT_MAX);

[Severity: High]
Can this truncation bypass the minimum floor limit?

If rate exceeds the 32-bit maximum (for example, during very high
bandwidth requests), the cast to u32 in min_t() will truncate the upper
bits. This could result in a near-zero clock rate instead of capping it,
potentially freezing the interconnect bus.

[ ... ]
> +static struct qcom_rpm *msm8660_get_rpm(struct device *dev)
> +{
[ ... ]
> +	rpm_pdev = of_find_device_by_node(rpm_np);
> +	of_node_put(rpm_np);
> +	if (!rpm_pdev) {
> +		dev_dbg(dev, "RPM device not found yet, deferring probe\n");
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	rpm = dev_get_drvdata(&rpm_pdev->dev);

[Severity: High]
Could this lead to a race condition between retrieving the pointer and
pinning the device?

Because dev_get_drvdata() retrieves the pointer before device_link_add()
pins the supplier, a concurrent unbind and rebind of the supplier in this
window could cause the original pointer to be freed, even though the link
might succeed on the rebound instance, leading to a use-after-free.

[ ... ]
> +static int msm8660_icc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	if (qp->num_clks) {
> +		ret = clk_bulk_prepare_enable(qp->num_clks, qp->bus_clks);
> +		if (ret) {
> +			dev_warn(dev, "Failed to enable bus clocks: %d\n", ret);
> +			qp->num_clks = 0;
> +		}
> +	}
> +
> +	/* Set up RPM fabric arbitration */
> +	qp->desc = desc;
> +	if (desc->rpm_resource >= 0) {
> +		qp->rpm = msm8660_get_rpm(dev);
> +		if (IS_ERR(qp->rpm))
> +			return PTR_ERR(qp->rpm);

[Severity: High]
Does returning directly here leak the clock prepare/enable reference?

When msm8660_get_rpm() returns -EPROBE_DEFER, returning directly skips
the rollback for the clk_bulk_prepare_enable() call made earlier. This
will leak the clock reference on every probe retry.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780197411.git.github.com@herrie.org?part=2