Re: [PATCH net-next v4 2/2] net: wwan: qcom_bam_dmux: Assign restricted DMA pool to remote processor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: wwan: qcom_bam_dmux: race between bam_dmux_pc_irq() and bam_dmux_probe()
- [Low] net: wwan: qcom_bam_dmux: incomplete vmid validation in bam_dmux_assign_dma_pool()
--

commit e5defad9cc621641da7903957a89aa1c3fa0145a
Author: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>

net: wwan: qcom_bam_dmux: Assign restricted DMA pool to remote processor

This commit assigns a restricted DMA pool to both the local and remote
execution environments via QCOM SCM when the qcom,vmid property is present
in the device tree, and reclaims the pool for the local environment during
probe cleanup or driver removal if the remote side has dropped its power
vote.

> diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
> index cc6ace8d64371..c9639474e58ac 100644
> --- a/drivers/net/wwan/qcom_bam_dmux.c
> +++ b/drivers/net/wwan/qcom_bam_dmux.c

[ ... ]

> @@ -762,6 +768,96 @@ static int __maybe_unused bam_dmux_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int bam_dmux_assign_dma_pool(struct bam_dmux *dmux)
> +{
> +	struct device *dev = dmux->dev;
> +	struct device_node *rmem_np;
> +	struct reserved_mem *rmem;
> +	struct qcom_scm_vmperm dst[2];
> +	u64 src = BIT_ULL(QCOM_SCM_VMID_HLOS);
> +	u32 vmid;
> +	int ret;
> +
> +	if (!of_property_present(dev->of_node, "qcom,vmid"))
> +		return 0;
> +
> +	ret = of_property_read_u32(dev->of_node, "qcom,vmid", &vmid);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read qcom,vmid\n");
> +	if (vmid == QCOM_SCM_VMID_HLOS || vmid >= BITS_PER_TYPE(u64))
> +		return dev_err_probe(dev, -EINVAL, "Invalid qcom,vmid %u\n", vmid);

[Severity: Low]
As reported by Sashiko on v2 [1], in Documentation/devicetree/bindings/net/qcom,bam-dmux.yaml,
qcom,vmid specifies minimum: 1 and maximum: 63, since 0 is not a valid
Qualcomm SCM VMID. However, bam_dmux_assign_dma_pool() does not reject
vmid == 0 and passes it on to qcom_scm_assign_mem().

Also, when bam_dmux_assign_dma_pool() records dmux->dma_pool_perms using
BIT_ULL(vmid) and bam_dmux_reclaim_dma_pool() passes it as src to
qcom_scm_assign_mem(), qcom_scm_assign_mem() tests srcvm_bits with 32-bit
BIT(b):

drivers/firmware/qcom/qcom_scm.c:qcom_scm_assign_mem() {
	...
	for (b = 0; b < BITS_PER_TYPE(u64); b++) {
		if (srcvm_bits & BIT(b))
			src[i++] = cpu_to_le32(b);
	}
	...
	for (i = 0; i < dest_cnt; i++, destvm++, newvm++) {
		...
		next_vm |= BIT(newvm->vmid);
	}
}

On 32-bit kernels (supported by QCOM_BAM_DMUX), BIT(b) is a 32-bit unsigned
long shift, so vmid values in 32..63 trigger shift undefined behavior and
fail to populate src[] properly during reclaim.

Should bam_dmux_assign_dma_pool() reject vmid == 0 (or enforce a tighter
upper bound unless qcom_scm_assign_mem() uses BIT_ULL())?

[ ... ]

> @@ -829,8 +929,11 @@ static int bam_dmux_probe(struct platform_device *pdev)
>  
>  	ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL,
>  				    &dmux->pc_state);
> -	if (ret)
> -		goto err_disable_pm;
> +	if (ret) {
> +		/* The PC IRQ may already have powered up the RX path */
> +		bam_dmux_remove(pdev);
> +		return ret;
> +	}
>  
>  	/* Check if remote finished initialization before us */
>  	if (dmux->pc_state) {
> @@ -845,6 +948,7 @@ static int bam_dmux_probe(struct platform_device *pdev)
>  err_disable_pm:
>  	pm_runtime_disable(dev);
>  	pm_runtime_dont_use_autosuspend(dev);
> +	bam_dmux_reclaim_dma_pool(dmux);
>  	return ret;
>  }
>  
> @@ -879,6 +983,12 @@ static void bam_dmux_remove(struct platform_device *pdev)
>  	disable_irq(dmux->pc_irq);
>  	bam_dmux_power_off(dmux);
>  	bam_dmux_free_skbs(dmux->tx_skbs, DMA_TO_DEVICE);
> +
> +	/* The remote side may still access the pool if it holds its power vote */
> +	if (!dmux->pc_state)
> +		bam_dmux_reclaim_dma_pool(dmux);
> +	else if (dmux->dma_pool_perms)
> +		dev_warn(dev, "Leaving restricted DMA pool assigned to remote\n");
>  }

[Severity: High]
Can bam_dmux_pc_irq() race with bam_dmux_probe() after devm_request_threaded_irq()
enables dmux->pc_irq, corrupting dmux->pc_state and causing
bam_dmux_remove() to prematurely reclaim the DMA pool or leak RX DMA
resources?

In bam_dmux_probe(), devm_request_threaded_irq() enables dmux->pc_irq before
irq_get_irqchip_state() and the initial dmux->pc_state check:

	ret = devm_request_threaded_irq(dev, dmux->pc_irq, NULL, bam_dmux_pc_irq,
					IRQF_ONESHOT, NULL, dmux);
	if (ret)
		goto err_disable_pm;

	ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL,
				    &dmux->pc_state);
	if (ret) {
		/* The PC IRQ may already have powered up the RX path */
		bam_dmux_remove(pdev);
		return ret;
	}

	/* Check if remote finished initialization before us */
	if (dmux->pc_state) {
		if (bam_dmux_power_on(dmux))
			bam_dmux_pc_ack(dmux);
		else
			bam_dmux_power_off(dmux);
	}

Meanwhile, bam_dmux_pc_irq() toggles dmux->pc_state without synchronization:

	bool new_state = !dmux->pc_state;
	...
	if (new_state) {
		if (bam_dmux_power_on(dmux))
			bam_dmux_pc_ack(dmux);
		else
			bam_dmux_power_off(dmux);
	} else {
		bam_dmux_power_off(dmux);
		bam_dmux_pc_ack(dmux);
	}

	dmux->pc_state = new_state;

If the remote side asserts the PC line right after devm_request_threaded_irq()
unmasks dmux->pc_irq:

1. If irq_get_irqchip_state() sets dmux->pc_state = true and bam_dmux_probe()
   powers on RX before bam_dmux_pc_irq() runs for that rising edge,
   bam_dmux_pc_irq() computes new_state = !dmux->pc_state (false), powers
   off RX, and sets dmux->pc_state = false (or irq_get_irqchip_state() can
   concurrently overwrite dmux->pc_state with false after bam_dmux_pc_irq()
   sets it to true). When bam_dmux_remove() later checks !dmux->pc_state, it
   will call bam_dmux_reclaim_dma_pool(dmux) while the remote processor
   still holds its power vote.

2. If bam_dmux_pc_irq() and bam_dmux_probe() both see the rising edge and
   call bam_dmux_power_on(dmux), the second dma_request_chan(dev, "rx")
   fails with -EBUSY and sets dmux->rx = NULL (leaking the active RX DMA
   channel), and bam_dmux_power_off(dmux) then skips
   dmaengine_terminate_sync() and unmaps and frees dmux->rx_skbs[] while RX
   DMA is still active.

3. On the new error path where irq_get_irqchip_state() fails and calls
   bam_dmux_remove(pdev), wait_event_timeout(dmux->pc_wait, !dmux->rx, ...)
   runs before disable_irq(dmux->pc_irq). If bam_dmux_pc_irq() has started
   on another CPU but has not yet assigned dmux->rx, wait_event_timeout()
   returns immediately while dmux->rx is still NULL, disable_irq() waits for
   bam_dmux_pc_irq() to finish powering on RX and sending bam_dmux_pc_ack(),
   and bam_dmux_power_off() immediately tears down RX without waiting for
   the remote side to drop its power vote.

Would requesting dmux->pc_irq with IRQF_NO_AUTOEN, reading the initial
irq_get_irqchip_state() and powering on if needed, and only then calling
enable_irq(dmux->pc_irq) avoid these races?

[1] https://lore.kernel.org/r/178996250389.2160803.17277339384338088713@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-qcom-bam-dmux-vmid-ext-v4-0-3690cc5ca0fd@oss.qualcomm.com?part=2