Re: [PATCH net v5] net: wwan: qcom_bam_dmux: fix TX DMA channel use-after-free

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (2) · Medium: 1 · Low: 0

- [High] The new design (live level re-read via
  irq_get_irqchip_state(IRQCHIP_STATE_LINE_LEVEL) as the only state…
- [Medium] bam_dmux_runtime_suspend() turns a possibly stale
  BAM_DMUX_PC_VOTE_DOWN_PENDING into an irreversible power-down…

Pre-existing issues:
- [High] bam_dmux_remove() cancels register_netdev_work and unregisters
  all netdevs while the RX DMA producer and the pc IRQ are still live;…
- [High] bam_dmux_rx_callback() can run concurrently with
  bam_dmux_power_off() and dereference a released/NULL RX channel.

--

net: wwan: qcom_bam_dmux: fix TX DMA channel use-after-free

This adds a power_lock mutex to serialize the modem power-control state,
re-reads the pc line level at each decision point instead of toggling a
cached bool, defers a pc=false transition while the host vote is active and
TX is allocated, and keeps both state IRQs masked with IRQF_NO_AUTOEN until
probe has committed the initial state.

Link: https://lore.kernel.org/netdev/20260822085308.1089-1-getshell@seu.edu.cn/

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

[ ... ]

> @@ -655,6 +683,8 @@ static void bam_dmux_free_skbs(struct bam_dmux_skb_dma skbs[],
>  
>  static void bam_dmux_power_off(struct bam_dmux *dmux)
>  {
> +	lockdep_assert_held(&dmux->power_lock);
> +
>  	if (dmux->tx) {
>  		dmaengine_terminate_sync(dmux->tx);
>  		dma_release_channel(dmux->tx);

[Severity: High]
This isn't a bug introduced by this patch, but now that power_lock
serializes the power-control paths, is the RX completion callback still
left outside that serialization?

bam_dmux_rx_callback() ends with an unconditional refill:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_rx_callback() {
	...
out:
	if (bam_dmux_skb_dma_queue_rx(skb_dma, GFP_ATOMIC))
		dma_async_issue_pending(dmux->rx);
}

while bam_dmux_power_off() does:

	if (dmux->rx) {
		dmaengine_terminate_sync(dmux->rx);
		dma_release_channel(dmux->rx);
		dmux->rx = NULL;
	}

Can the refill see dmux->rx == NULL and oops? dmaengine_prep_config_single()
dereferences chan with no NULL check:

include/linux/dmaengine.h:
	if (chan->device->device_prep_config_sg)

Qualcomm BAM registers device_terminate_all but no device_synchronize:

drivers/dma/qcom/bam_dma.c:
	bdev->common.device_terminate_all = bam_dma_terminate_all;

so dmaengine_terminate_sync() -> dmaengine_synchronize() cannot wait for a
callback that vchan_complete() has already spliced off and started. Should
the RX callback also take power_lock, or check a shutdown flag, before
requeueing?

> @@ -670,10 +700,24 @@ static void bam_dmux_free_skbs(struct bam_dmux_skb_dma skbs[],
>  	bam_dmux_free_skbs(dmux->rx_skbs, DMA_FROM_DEVICE);
>  }
>  
> -static irqreturn_t bam_dmux_pc_irq(int irq, void *data)
> +static void bam_dmux_complete_pc_down(struct bam_dmux *dmux)
>  {
> -	struct bam_dmux *dmux = data;
> -	bool new_state = !dmux->pc_state;
> +	lockdep_assert_held(&dmux->power_lock);
> +
> +	bam_dmux_power_off(dmux);
> +	bam_dmux_pc_ack(dmux);
> +	WRITE_ONCE(dmux->pc_state, false);
> +}
> +
> +static bool bam_dmux_handle_pc(struct bam_dmux *dmux, bool new_state)
> +{
> +	lockdep_assert_held(&dmux->power_lock);
> +
> +	if (new_state == dmux->pc_state) {
> +		if (new_state && dmux->pc_vote == BAM_DMUX_PC_VOTE_DOWN_PENDING)
> +			dmux->pc_vote = BAM_DMUX_PC_VOTE_ACTIVE;
> +		return false;
> +	}
>  
>  	dev_dbg(dmux->dev, "pc: %u\n", new_state);
>  

[ ... ]

> @@ -706,9 +795,47 @@ static irqreturn_t bam_dmux_pc_ack_irq(int irq, void *data)
>  static int bam_dmux_runtime_suspend(struct device *dev)
>  {
>  	struct bam_dmux *dmux = dev_get_drvdata(dev);
> +	bool wake = false;
>  
>  	dev_dbg(dev, "runtime suspend\n");
> -	bam_dmux_pc_vote(dmux, false);
> +
> +	scoped_guard(mutex, &dmux->power_lock) {
> +		bam_dmux_pc_vote_locked(dmux, false);
> +		if (dmux->pc_vote == BAM_DMUX_PC_VOTE_DOWN_PENDING) {
> +			dmux->pc_vote = BAM_DMUX_PC_VOTE_INACTIVE;
> +			bam_dmux_complete_pc_down(dmux);
> +			wake = true;
> +		} else if (!dmux->pc_state) {
> +			bam_dmux_power_off(dmux);
> +		}
> +	}

[Severity: Medium]
Is it safe for this to act on the cached BAM_DMUX_PC_VOTE_DOWN_PENDING
without re-sampling the pc line?

Every other new decision point in the patch calls
irq_get_irqchip_state(IRQCHIP_STATE_LINE_LEVEL) first, but this one
converts the cached flag straight into bam_dmux_complete_pc_down(), which
releases both channels, frees the RX skbs and toggles pc-ack:

	bam_dmux_power_off(dmux);
	bam_dmux_pc_ack(dmux);
	WRITE_ONCE(dmux->pc_state, false);

DOWN_PENDING is only cancelled from bam_dmux_handle_pc() when a later
invocation observes new_state == dmux->pc_state == true:

	if (new_state == dmux->pc_state) {
		if (new_state && dmux->pc_vote == BAM_DMUX_PC_VOTE_DOWN_PENDING)
			dmux->pc_vote = BAM_DMUX_PC_VOTE_ACTIVE;

So if the modem re-raises pc and the rising-edge handler is still blocked on
mutex_lock(&dmux->power_lock) when the autosuspend timer fires:

bam_dmux_runtime_suspend()
   scoped_guard(mutex, &dmux->power_lock)
      bam_dmux_complete_pc_down()   /* pc line is high again */

does the driver then acknowledge a pc=false transition the modem has already
revoked, and tear down RX while the peer believes the link is up? The commit
message describes this as the case to avoid:

    "acknowledging a new pc=false transition while the host vote remains
    active lets the modem power down the DMA engine underneath the driver"

Would reading the line level here before calling
bam_dmux_complete_pc_down() close that window?

[ ... ]

> @@ -817,31 +939,40 @@ static int bam_dmux_probe(struct platform_device *pdev)
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_enable(dev);
>  
> -	ret = devm_request_threaded_irq(dev, pc_ack_irq, NULL, bam_dmux_pc_ack_irq,
> -					IRQF_ONESHOT, NULL, dmux);
> +	ret = devm_request_threaded_irq(dev, dmux->pc_ack_irq, NULL,
> +					bam_dmux_pc_ack_irq,
> +					IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +					NULL, dmux);
>  	if (ret)
>  		goto err_disable_pm;
>  
>  	ret = devm_request_threaded_irq(dev, dmux->pc_irq, NULL, bam_dmux_pc_irq,
> -					IRQF_ONESHOT, NULL, dmux);
> +					IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, dmux);
>  	if (ret)
> -		goto err_disable_pm;
> +		goto err_power_off;
>  
> +	mutex_lock(&dmux->power_lock);
>  	ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL,
> -				    &dmux->pc_state);
> -	if (ret)
> -		goto err_disable_pm;
> +				    &pc_state);
> +	if (ret) {
> +		mutex_unlock(&dmux->power_lock);
> +		goto err_power_off;
> +	}
>  
>  	/* 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);
> -	}
> +	bam_dmux_handle_pc(dmux, pc_state);
> +	mutex_unlock(&dmux->power_lock);
> +
> +	/* Start handling state changes after the initial state is processed. */
> +	enable_irq(dmux->pc_ack_irq);
> +	enable_irq(dmux->pc_irq);

[Severity: High]
Can a pc transition be lost in the window created here between the level
snapshot and enable_irq(dmux->pc_irq)?

The in-tree DTs for qcom,bam-dmux (msm8916.dtsi, msm8939.dtsi) parent these
interrupts on SMSM:

	interrupt-parent = <&hexagon_smsm>;
	interrupts = <1 IRQ_TYPE_EDGE_BOTH>, <11 IRQ_TYPE_EDGE_BOTH>;

and smsm_intr() advances the cached level for all bits but only dispatches
the enabled ones:

drivers/soc/qcom/smsm.c:smsm_intr() {
	val = readl(entry->remote_state);
	changed = val ^ xchg(&entry->last_value, val);

	for_each_set_bit(i, entry->irq_enabled, 32) {
	...
}

and on unmask the cache is re-synced to the live line rather than replaying
the missed edge:

drivers/soc/qcom/smsm.c:smsm_unmask_irq() {
	/* Make sure our last cached state is up-to-date */
	if (readl(entry->remote_state) & BIT(irq))
		set_bit(irq, &entry->last_value);
	else
		clear_bit(irq, &entry->last_value);
}

The smp2p provider does the same thing in qcom_smp2p_notify_in().

So if the modem raises pc after irq_get_irqchip_state() but before
enable_irq(), is that edge simply consumed?  bam_dmux_power_on() would never
run, no pc-ack would be toggled, and the following falling edge would land in
bam_dmux_handle_pc() with new_state == dmux->pc_state == false:

	if (new_state == dmux->pc_state) {
		...
		return false;
	}

after which every bam_dmux_runtime_resume() would time out in

	wait_event_timeout(dmux->pc_wait, READ_ONCE(dmux->pc_state),
			   BAM_DMUX_REMOTE_TIMEOUT)

because the modem has no reason to produce another rising edge. Given that
probe already has a "Check if remote finished initialization before us" path,
a pc rising edge concurrent with probe looks like expected timing. Is there a
recovery path for that case, or does relying on a re-read of the line level
while the child IRQ is masked need a different approach?

>  
>  	return 0;
>  
> +err_power_off:
> +	mutex_lock(&dmux->power_lock);
> +	bam_dmux_power_off(dmux);
> +	mutex_unlock(&dmux->power_lock);
>  err_disable_pm:
>  	pm_runtime_disable(dev);
>  	pm_runtime_dont_use_autosuspend(dev);
> @@ -872,12 +1003,16 @@ 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))
> +	if (!wait_event_timeout(dmux->pc_wait, !READ_ONCE(dmux->rx),
> +				BAM_DMUX_REMOTE_TIMEOUT))
>  		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_ack_irq);
>  	disable_irq(dmux->pc_irq);
> +	mutex_lock(&dmux->power_lock);
>  	bam_dmux_power_off(dmux);
> +	mutex_unlock(&dmux->power_lock);
>  	bam_dmux_free_skbs(dmux->tx_skbs, DMA_TO_DEVICE);
>  }

[Severity: High]
This is a pre-existing ordering issue rather than something the patch
introduces, but since the remove path is being touched here: can
register_netdev_work be re-queued after its only cancellation?

bam_dmux_remove() runs cancel_work_sync(&dmux->register_netdev_work) and
unregister_netdevice_many() at the top, while the RX channel is still armed
and the pc IRQ is still enabled. Only afterwards does it reach
disable_irq()/bam_dmux_power_off().

A remote OPEN command arriving in that window does:

bam_dmux_rx_callback()
  bam_dmux_cmd_open()
     /* Cannot sleep here, schedule work to register the netdev */
     schedule_work(&dmux->register_netdev_work);

and there is no second cancel_work_sync() after the IRQs are disabled and the
DMA is terminated. bam_dmux_register_netdev_work() would then dereference the
devm-allocated dmux and publish a fresh netdev holding it:

	bndev = netdev_priv(netdev);
	bndev->dmux = dmux;
	...
	ret = register_netdev(netdev);

Once remove() returns, devm frees dmux, so any xmit on that interface would
touch freed memory. Would moving the cancel_work_sync() after
disable_irq()/bam_dmux_power_off(), or adding a second one there, be
appropriate?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903180057.1437-1-getshell%40seu.edu.cn