Re: [PATCH 2/2] net: wwan: qcom_bam_dmux: Alloc RX buffers as a single coherent block

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Double fetch / TOCTOU vulnerability due to reading validated BAM-DMUX RX header fields directly from shared coherent memory.
- [High] The driver passes a `dma_addr_t` to `qcom_scm_assign_mem()`, which expects a `phys_addr_t`, causing memory protection failures when an IOMMU is active.
- [Low] Unconditional 'select QCOM_SCM' in a driver that supports COMPILE_TEST causes unmet direct dependency warnings on unsupported architectures.

Pre-existing issues:
- [High] The probe error path leaves the pc_irq enabled, which can trigger unhandled DMA operations that continue running after the dmux struct is freed, causing a Use-After-Free.
--

--- Patch [2]: [PATCH 2/2] net: wwan: qcom_bam_dmux: Alloc RX buffers as a single coherent block ---
commit 1922e77e47093401dce237ccfcb72b740d813974
Author: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>

net: wwan: qcom_bam_dmux: Alloc RX buffers as a single coherent block

This patch modifies the BAM-DMUX driver to allocate all RX buffers as a single
contiguous DMA coherent block on supported platforms. This reduces the number
of required XPU resource groups from 32 to 1, avoiding XPU violations on newer
Qualcomm SoCs.

> diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
> index 958dbc7347fa8..1b133c5623161 100644
> --- a/drivers/net/wwan/Kconfig
> +++ b/drivers/net/wwan/Kconfig
> @@ -65,6 +65,7 @@ config MHI_WWAN_MBIM
>  config QCOM_BAM_DMUX
>  	tristate "Qualcomm BAM-DMUX WWAN network driver"
>  	depends on (DMA_ENGINE && PM && QCOM_SMEM_STATE) || COMPILE_TEST
> +	select QCOM_SCM

[Severity: Low]
Does selecting an architecture-specific symbol here cause unmet direct
dependency warnings on unsupported architectures? Since this driver
supports COMPILE_TEST, building it on x86 or other non-ARM architectures
could trigger warnings if QCOM_SCM's own dependencies are not met.

[ ... ]

> diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
> index cc6ace8d64371..247230b720e60 100644
> --- a/drivers/net/wwan/qcom_bam_dmux.c
> +++ b/drivers/net/wwan/qcom_bam_dmux.c
> @@ -499,9 +521,10 @@ static bool bam_dmux_skb_dma_queue_rx(struct bam_dmux_skb_dma *skb_dma, gfp_t gf
>  static void bam_dmux_cmd_data(struct bam_dmux_skb_dma *skb_dma)
>  {
>  	struct bam_dmux *dmux = skb_dma->dmux;
> -	struct sk_buff *skb = skb_dma->skb;
> -	struct bam_dmux_hdr *hdr = (struct bam_dmux_hdr *)skb->data;
> +	struct bam_dmux_hdr *hdr = skb_dma->rx_virt ? skb_dma->rx_virt :
> +				   (struct bam_dmux_hdr *)skb_dma->skb->data;
>  	struct net_device *netdev = dmux->netdevs[hdr->ch];
> +	struct sk_buff *skb;

[Severity: Critical]
Is there a double-fetch vulnerability introduced by reading the header fields
directly from the shared coherent RX block?

Earlier in bam_dmux_rx_callback(), hdr->ch and hdr->len were validated.
However, because the memory is shared and remains writable by the modem,
a malicious remote processor could overwrite hdr->ch or hdr->len after the
initial checks but before they are re-read here to index the dmux->netdevs
array and allocate the SKB.

Could this allow an out-of-bounds read or memory corruption if the modem
alters the header mid-flight? Should the header be copied to local kernel
memory before validation?

[ ... ]

> @@ -762,6 +800,71 @@ static int __maybe_unused bam_dmux_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int bam_dmux_alloc_coherent_rx(struct bam_dmux *dmux)
> +{
> +	struct device *dev = dmux->dev;
> +	const struct bam_dmux_data *data = of_device_get_match_data(dev);
> +	size_t size = BAM_DMUX_NUM_SKB * BAM_DMUX_BUFFER_SIZE;
> +	u64 src = BIT_ULL(QCOM_SCM_VMID_HLOS);
> +	struct qcom_scm_vmperm dst[2];
> +	int i, ret;
> +
> +	if (!data)
> +		return 0;
> +
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
> +	dst[0].vmid = QCOM_SCM_VMID_HLOS;
> +	dst[0].perm = QCOM_SCM_PERM_RW;
> +	dst[1].vmid = data->vmid;
> +	dst[1].perm = QCOM_SCM_PERM_RW;
> +
> +	dmux->rx_buf = dma_alloc_coherent(dev, size, &dmux->rx_buf_dma, GFP_KERNEL);
> +	if (!dmux->rx_buf)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < BAM_DMUX_NUM_SKB; i++) {
> +		dmux->rx_skbs[i].rx_virt = dmux->rx_buf + i * BAM_DMUX_BUFFER_SIZE;
> +		dmux->rx_skbs[i].addr = dmux->rx_buf_dma + i * BAM_DMUX_BUFFER_SIZE;
> +	}
> +
> +	ret = qcom_scm_assign_mem(dmux->rx_buf_dma, size, &src, dst, ARRAY_SIZE(dst));

[Severity: High]
Does qcom_scm_assign_mem() expect a physical address rather than a DMA
address?

Here, dmux->rx_buf_dma is a dma_addr_t returned by dma_alloc_coherent().
If an IOMMU is active, this is an IOVA. Passing an IOVA to the TrustZone
XPU instead of a physical address could result in incorrect memory
protection, potentially causing an SCM fault or a system crash when the
modem attempts to access the buffers.

[ ... ]

> @@ -845,6 +952,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_free_coherent_rx(dmux);
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does this error path need to disable
the pc_irq?

If bam_dmux_probe() successfully requests pc_irq via
devm_request_threaded_irq() but a subsequent step fails, it jumps to
err_disable_pm and returns without disabling the IRQ.

Could this cause a use-after-free if the modem triggers the IRQ before
devres frees it? The IRQ handler could run, start DMA operations, and
then have the memory freed out from under it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-qcom-bam-dmux-vmid-ext-v1-0-3f29da7cca76@oss.qualcomm.com?part=2