Re: [PATCH v2 2/3] soundwire: qcom: add multi-master support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] soundwire: qcom: use-after-free in qcom_swrm_multi_master_setup due to lockless drvdata access
- [High] soundwire: qcom: out-of-bounds MMIO access due to unchecked port_num underflow
- [High] soundwire: qcom: clock double-disable and unclocked access risk in remove path
- [Medium] soundwire: qcom: interface clock leaked for dependent multi-master device
--

commit c7571d3da0c003125e279a9926c2184e7157d267
Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
soundwire: qcom: add multi-master support

This commit adds support for v3.1.0+ Qualcomm SoundWire IPs that are paired
to expose one logical bus with more than two DATA lanes. It introduces a
multi-master mode where one primary IP owns SWR_CLK and handles commands,
while the dependent IP provides data lanes only. A device link enforces
unbind ordering between the two controllers.

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 35ffffd541bd1..767147a6635dc 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
[ ... ]
> @@ -1040,9 +1051,41 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
[ ... ]
> +static struct qcom_swrm_ctrl *
> +qcom_swrm_port_target(struct qcom_swrm_ctrl *ctrl, u8 port_num, u32 *reg)
> +{
> +	u8 lane = ctrl->pconfig[port_num].lane_control;
> +
> +	if (ctrl->peer_ctrl && lane >= ctrl->peer_first_lane &&
> +	    lane < ctrl->peer_first_lane + ctrl->num_peer_lanes) {
> +		*reg -= ctrl->peer_dpn_offset;

[Severity: High]
Is there a risk of an integer underflow here resulting in an out-of-bounds
MMIO access?

If the device tree maps a logical port (port_num) where the computed register
address is smaller than the offset parameter passed via
qcom,multi-master-peer, *reg would underflow.

This wrapped address would then be used in reg_write, writing directly to
out-of-bounds MMIO. Does this subtraction require bounds checking?

> +		return ctrl->peer_ctrl;
> +	}
> +	return ctrl;
> +}
[ ... ]
> @@ -1562,6 +1610,73 @@ static int swrm_reg_show(struct seq_file *s_file, void *data)
[ ... ]
> +static int qcom_swrm_multi_master_setup(struct qcom_swrm_ctrl *ctrl)
> +{
> +	struct of_phandle_args args;
> +	struct qcom_swrm_ctrl *peer;
> +	struct platform_device *pdev;
> +	u32 offset;
> +	int ret;
[ ... ]
> +	pdev = of_find_device_by_node(args.np);
> +	of_node_put(args.np);
> +	if (!pdev)
> +		return -EPROBE_DEFER;
[ ... ]
> +	peer = platform_get_drvdata(pdev);
> +	if (!peer || !peer->is_dependent) {

[Severity: High]
Can evaluating peer->is_dependent here lead to a use-after-free?

platform_get_drvdata(pdev) is called without holding device_lock(&pdev->dev).
If a concurrent sysfs unbind occurs on the peer device, devres could free the
qcom_swrm_ctrl structure right before dev_set_drvdata(dev, NULL) is called
in the driver core teardown.

This creates a window where peer points to freed memory when evaluating
peer->is_dependent.

> +		ret = -EPROBE_DEFER;
> +		goto out;
> +	}
[ ... ]
> @@ -1689,6 +1817,16 @@ static int qcom_swrm_probe(struct platform_device *pdev)
[ ... ]
> +	if (of_property_match_string(dev->of_node, "qcom,multi-master-mode", "dependent") >= 0) {
> +		ctrl->is_dependent = true;
> +		dev_dbg(dev,
> +			"Qualcomm SoundWire multi-master dependent v%x.%x.%x registered (%u lanes)\n",
> +			(ctrl->version >> 24) & 0xff,
> +			(ctrl->version >> 16) & 0xff,
> +			ctrl->version & 0xffff, ctrl->num_lanes);
> +		return 0;

[Severity: Medium]
Does this early return permanently strand the interface clock in a powered-on
state?

Because probe returns 0 early for dependent devices, it bypasses
pm_runtime_enable(). Since probe already called
clk_prepare_enable(ctrl->hclk), the clock is never disabled as
swrm_runtime_suspend will never be invoked for the dependent device.

If the clock is shared with the primary device, the dependent's reference
keeps the physical clock constantly on.

> +	}
> +
>  	ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode);
[ ... ]
> @@ -1733,7 +1886,21 @@ static void qcom_swrm_remove(struct platform_device *pdev)
>  {
>  	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
>  
> -	sdw_bus_master_delete(&ctrl->bus);
> +	/* delete bus before dropping peer_ctrl; in-flight ops deref it */
> +	if (!ctrl->is_dependent)
> +		sdw_bus_master_delete(&ctrl->bus);
> +
> +	if (ctrl->is_primary && ctrl->peer_ctrl) {
> +		struct qcom_swrm_ctrl *peer = ctrl->peer_ctrl;
> +
> +		peer->reg_write(peer,
> +				peer->reg_layout[SWRM_REG_INTERRUPT_CPU_EN], 0);

[Severity: High]
Does this risk unclocked hardware access or unbalanced clock disables?

qcom_swrm_remove executes these teardown writes and the subsequent
clk_disable_unprepare without first calling pm_runtime_get_sync(ctrl->dev).
Since the driver core may call remove while the device is runtime suspended
(where swrm_runtime_suspend has already disabled hclk), writing to peer
registers during this suspended state may cause interconnect stalls.

Furthermore, clk_disable_unprepare(ctrl->hclk) gets called twice, causing a
clock underflow warning from the clock framework.

> +		/* disable peer SWRM; releases shared SWR_CLK */
> +		peer->reg_write(peer, SWRM_COMP_CFG_ADDR, 0);
> +		peer->peer_ctrl = NULL;
> +		ctrl->peer_ctrl = NULL;
> +	}
> +
>  	clk_disable_unprepare(ctrl->hclk);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001093806.634862-1-srinivas.kandagatla@oss.qualcomm.com?part=2