Re: [PATCH net-next v3 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 on remote_suspended in bam_dmux_remove()
- [High] net: wwan: qcom_bam_dmux: UAF on register_netdev_work in bam_dmux_remove()
--

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

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

This commit adds support for assigning a restricted DMA pool to both the
local and remote execution environments via SCM when qcom,vmid is present,
and reclaiming the pool for the local environment during probe failure or
driver removal if the remote processor has suspended.

> diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
> index cc6ace8d64371..fa3621224a56e 100644
> --- a/drivers/net/wwan/qcom_bam_dmux.c
> +++ b/drivers/net/wwan/qcom_bam_dmux.c
[ ... ]
> @@ -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;
> +	}

[Severity: High]
When irq_get_irqchip_state() fails in bam_dmux_probe() in
drivers/net/wwan/qcom_bam_dmux.c (or during normal driver removal),
calling bam_dmux_remove() cancels register_netdev_work and unregisters
existing netdevs while dmux->pc_irq and the RX DMA channel can still be
active:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_remove() {
	/* Unregister network interfaces */
	cancel_work_sync(&dmux->register_netdev_work);
	rtnl_lock();
	for (i = 0; i < BAM_DMUX_NUM_CH; ++i)
		if (dmux->netdevs[i])
			unregister_netdevice_queue(dmux->netdevs[i], &list);
	unregister_netdevice_many(&list);
	rtnl_unlock();
	...
	/* Try to wait for remote side to drop power vote */
	remote_suspended = wait_event_timeout(dmux->pc_wait, !dmux->rx,
					      BAM_DMUX_REMOTE_TIMEOUT);
	...
	/* Make sure everything is cleaned up before we return */
	disable_irq(dmux->pc_irq);
	bam_dmux_power_off(dmux);
}

While bam_dmux_remove() is waiting in wait_event_timeout(), incoming
BAM_DMUX_CMD_OPEN packets on the active RX DMA path invoke
bam_dmux_rx_callback() -> bam_dmux_cmd_open(), which schedules
dmux->register_netdev_work again:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_cmd_open() {
	if (netdev) {
		netif_device_attach(netdev);
	} else {
		/* Cannot sleep here, schedule work to register the netdev */
		schedule_work(&dmux->register_netdev_work);
	}
}

Because register_netdev_work is not canceled and netdevs are not
unregistered after disable_irq() and bam_dmux_power_off() stop the IRQ
and RX DMA, can bam_dmux_register_netdev_work() run after
bam_dmux_remove() returns, causing a use-after-free on the devm-allocated
struct bam_dmux or leaving registered net_device instances pointing to
freed memory?

[ ... ]
> @@ -853,6 +957,7 @@ static void bam_dmux_remove(struct platform_device *pdev)
>  	struct bam_dmux *dmux = platform_get_drvdata(pdev);
>  	struct device *dev = dmux->dev;
>  	LIST_HEAD(list);
> +	bool remote_suspended;
>  	int i;
>  
>  	/* Unregister network interfaces */
> @@ -872,13 +977,21 @@ static void bam_dmux_remove(struct platform_device *pdev)
>  	pm_runtime_set_suspended(dev);
>  
>  	/* Try to wait for remote side to drop power vote */
> -	if (!wait_event_timeout(dmux->pc_wait, !dmux->rx, BAM_DMUX_REMOTE_TIMEOUT))
> +	remote_suspended = wait_event_timeout(dmux->pc_wait, !dmux->rx,
> +					      BAM_DMUX_REMOTE_TIMEOUT);
> +	if (!remote_suspended)
>  		dev_err(dev, "Timed out waiting for remote side to suspend\n");
>  
>  	/* Make sure everything is cleaned up before we return */
>  	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 did not suspend */
> +	if (remote_suspended)
> +		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]
In bam_dmux_remove() in drivers/net/wwan/qcom_bam_dmux.c,
remote_suspended is sampled from wait_event_timeout(dmux->pc_wait,
!dmux->rx, BAM_DMUX_REMOTE_TIMEOUT) before disable_irq(dmux->pc_irq)
synchronizes with the threaded handler bam_dmux_pc_irq():

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_pc_irq() {
	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;
	wake_up_all(&dmux->pc_wait);
}

If dmux->rx is NULL when wait_event_timeout() checks !dmux->rx (for
example, if bam_dmux_pc_irq() was just triggered during the
bam_dmux_probe() error path or during removal and has not yet set
dmux->rx in bam_dmux_power_on()), wait_event_timeout() returns
immediately with remote_suspended set to true.

Before disable_irq(dmux->pc_irq) finishes waiting for the running IRQ
thread, bam_dmux_pc_irq() can complete bam_dmux_power_on(dmux) and send
bam_dmux_pc_ack(dmux) to the remote processor. Then bam_dmux_power_off()
tears down dmux->rx locally without a remote power-down handshake, and
bam_dmux_remove() still sees the stale remote_suspended == true and calls
bam_dmux_reclaim_dma_pool(dmux) while the remote side is active.

Conversely, if wait_event_timeout() times out while bam_dmux_pc_irq() is
in the middle of handling a remote power-down edge, disable_irq() waits
for bam_dmux_pc_irq() to finish calling bam_dmux_power_off() and
bam_dmux_pc_ack(), but remote_suspended remains false and the DMA pool
is never reclaimed.

Should remote_suspended be re-evaluated (for example, checking !dmux->rx)
after disable_irq(dmux->pc_irq) returns and before bam_dmux_power_off()
clears dmux->rx?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-qcom-bam-dmux-vmid-ext-v3-0-cedd1f681102@oss.qualcomm.com?part=2