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

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Sat, Aug 22, 2026 at 04:53:08PM +0800, Hongyan Xu wrote:
> The modem power-control interrupt can release dmux->tx while the command,
> netdev transmit, or deferred wakeup paths are preparing and submitting
> DMA descriptors. A runtime PM reference alone does not order those paths
> against the modem-driven interrupt.
> 
> The pc and pc-ack IRQ actions do not encode the modem protocol order. A
> pc=false edge raised before a new host power vote is acknowledged can
> therefore be handled after runtime resume observes the acknowledgment and
> starts using TX. On the other hand, acknowledging a new pc=false transition
> while the acknowledged host vote remains active lets the modem power down
> the DMA engine underneath the driver.
> 
> Serialize power-control state with a mutex and distinguish a requested host
> vote from an acknowledged one. Publish each vote acknowledgment and its
> completion under that lock. Before publishing the host-vote acknowledgment,
> sample the actual pc line and process any delayed transition. A valid
> pc=false transition is then handled before runtime resume returns, when no
> path can submit through TX. The channels are released before pc is
> acknowledged, and a queued BAM_DMUX_CMD_OPEN remains queued for the new TX
> channel.
> 
> If pc goes low after the host vote was acknowledged, refuse the pc
> acknowledgment and keep the DMA channels allocated. Remember the pending
> transition so runtime suspend can first release the channels and only then
> acknowledge pc=false. This preserves the protocol ordering without exposing
> a released TX channel or cancelling an OPEN command that already succeeded.
> 
> Use an enum for the host power-vote state, initialize the power mutex with
> devm_mutex_init(), and use scoped mutex guards in runtime PM paths. Let
> runtime suspend perform cleanup when runtime resume cannot acquire the TX
> channel. Keep both state IRQs disabled until the initial probe state is
> committed, and disable both before final remove cleanup.
> 
> This issue was found by the author's in-house static analysis tool.
> The patch was reviewed by the author.
> 
> Fixes: 21a0ffd9b38c ("net: wwan: Add Qualcomm BAM-DMUX WWAN network driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
> 
> ---
> Changes in v4:
> - Replace the power-vote booleans with an explicit enum state.
> - Remove the redundant RX-channel check and use devm_mutex_init().
> - Use scoped mutex guards and let runtime_suspend() clean up resume failures.
> - Keep both state IRQs disabled until probe initialization is complete.
> - Disable both state IRQs before final remove cleanup.
> 
> diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
> index cc6ace8..3756cdb 100644
> --- a/drivers/net/wwan/qcom_bam_dmux.c
> +++ b/drivers/net/wwan/qcom_bam_dmux.c
> [...]
> @@ -670,10 +701,15 @@ static void bam_dmux_power_off(struct bam_dmux *dmux)
>  	bam_dmux_free_skbs(dmux->rx_skbs, DMA_FROM_DEVICE);
>  }
>  
> -static irqreturn_t bam_dmux_pc_irq(int irq, void *data)
> +static bool bam_dmux_handle_pc(struct bam_dmux *dmux, bool new_state)
>  {
> -	struct bam_dmux *dmux = data;
> -	bool new_state = !dmux->pc_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_ACKED;
> +		return false;
> +	}
>  
>  	dev_dbg(dmux->dev, "pc: %u\n", new_state);
>  
> @@ -682,13 +718,41 @@ static irqreturn_t bam_dmux_pc_irq(int irq, void *data)
>  			bam_dmux_pc_ack(dmux);
>  		else
>  			bam_dmux_power_off(dmux);
> +	} else if (dmux->pc_vote == BAM_DMUX_PC_VOTE_ACKED) {
> +		/* The modem must keep the DMA engine on until pc is acked. */
> +		dmux->pc_vote = BAM_DMUX_PC_VOTE_DOWN_PENDING;
> +		dev_err_ratelimited(dmux->dev,
> +				    "refusing pc down while host vote is active\n");
> +		return false;

Sashiko notes here that the use after free is still present if

	dmux->pc_vote == BAM_DMUX_PC_VOTE_DOWN_PENDING

and the modem sends a spurious PC interrupt. The if statement for
entering this branch should probably include
BAM_DMUX_PC_VOTE_DOWN_PENDING so we don't accidentally overwrite it.
That by itself won't work though since bam_dmux_runtime_suspend() reuses
this function to clear the VOTE_DOWN_PENDING state...

https://sashiko.dev/#/patchset/20260822085308.1089-1-getshell%40seu.edu.cn

>  	} else {
>  		bam_dmux_power_off(dmux);
>  		bam_dmux_pc_ack(dmux);
> +		dmux->pc_vote = BAM_DMUX_PC_VOTE_INACTIVE;
>  	}
>  
> -	dmux->pc_state = new_state;
> -	wake_up_all(&dmux->pc_wait);
> +	WRITE_ONCE(dmux->pc_state, new_state);
> +	return true;
> +}
> +
> +static irqreturn_t bam_dmux_pc_irq(int irq, void *data)
> +{
> +	struct bam_dmux *dmux = data;
> +	bool new_state, wake;
> +	int ret;
> +
> +	mutex_lock(&dmux->power_lock);
> +	ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL,
> +				    &new_state);
> +	if (ret) {
> +		mutex_unlock(&dmux->power_lock);
> +		dev_err_ratelimited(dmux->dev, "failed to read pc state: %d\n", ret);
> +		return IRQ_HANDLED;
> +	}
> +
> +	wake = bam_dmux_handle_pc(dmux, new_state);
> +	mutex_unlock(&dmux->power_lock);
> +	if (wake)
> +		wake_up_all(&dmux->pc_wait);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -696,9 +760,30 @@ static irqreturn_t bam_dmux_pc_irq(int irq, void *data)
>  static irqreturn_t bam_dmux_pc_ack_irq(int irq, void *data)
>  {
>  	struct bam_dmux *dmux = data;
> +	bool new_state, wake = false;
> +	int ret;
>  
>  	dev_dbg(dmux->dev, "pc ack\n");
> +
> +	/* Process an earlier pc edge before publishing the host-vote ack. */
> +	synchronize_irq(dmux->pc_irq);
> +	mutex_lock(&dmux->power_lock);
> +	ret = irq_get_irqchip_state(dmux->pc_irq, IRQCHIP_STATE_LINE_LEVEL,
> +				    &new_state);
> +	if (ret) {
> +		mutex_unlock(&dmux->power_lock);
> +		dev_err_ratelimited(dmux->dev, "failed to read pc state: %d\n", ret);
> +		return IRQ_HANDLED;
> +	}
> +
> +	wake = bam_dmux_handle_pc(dmux, new_state);
> +	if (new_state && dmux->pc_state &&
> +	    dmux->pc_vote == BAM_DMUX_PC_VOTE_PENDING)
> +		dmux->pc_vote = BAM_DMUX_PC_VOTE_ACKED;

Sashiko notes here that "if the modem asserts the PC-ACK interrupt
before the PC line goes high, bam_dmux_pc_ack_irq() will see
dmux->pc_state as false. The vote will remain stuck in
BAM_DMUX_PC_VOTE_PENDING instead of transitioning to
BAM_DMUX_PC_VOTE_ACKED."

https://sashiko.dev/#/patchset/20260822085308.1089-1-getshell%40seu.edu.cn

I think Sashiko is right. The situation pointed out by Sashiko is even
the typical case, because normally the sequence is:

 1. Host requests pc vote=true (PC_VOTE_INACTIVE -> PC_VOTE_PENDING)
 2. Modem acks request
 3. Modem powers up hardware
 4. Modem indicates pc=true

Do we even need the BAM_DMUX_PC_VOTE_ACKED state? For bam_dmux_pc_irq()
I think the question at the end is if dmux->tx != NULL, or am I missing
something?

>  	complete_all(&dmux->pc_ack_completion);
> +	mutex_unlock(&dmux->power_lock);
> +	if (wake)
> +		wake_up_all(&dmux->pc_wait);
>  
>  	return IRQ_HANDLED;
>  }
> [...]
> @@ -724,50 +822,58 @@ static int __maybe_unused bam_dmux_runtime_resume(struct device *dev)
>  					 BAM_DMUX_REMOTE_TIMEOUT))
>  		return -ETIMEDOUT;
>  
> +	synchronize_irq(dmux->pc_irq);
> +
>  	/* Vote for power state */
> -	bam_dmux_pc_vote(dmux, true);
> +	bam_dmux_pc_vote_protected(dmux, true);
>  
>  	/* Wait for ack */
>  	if (!wait_for_completion_timeout(&dmux->pc_ack_completion,
>  					 BAM_DMUX_REMOTE_TIMEOUT)) {
> -		bam_dmux_pc_vote(dmux, false);
> +		bam_dmux_runtime_suspend(dev);
>  		return -ETIMEDOUT;
>  	}
>  
> +	synchronize_irq(dmux->pc_irq);
> +
>  	/* Wait until we're up */
> -	if (!wait_event_timeout(dmux->pc_wait, dmux->pc_state,
> +	if (!wait_event_timeout(dmux->pc_wait, READ_ONCE(dmux->pc_state),
>  				BAM_DMUX_REMOTE_TIMEOUT)) {
> -		bam_dmux_pc_vote(dmux, false);
> +		bam_dmux_runtime_suspend(dev);
>  		return -ETIMEDOUT;
>  	}
>  
> -	/* Ensure that we actually initialized successfully */
> -	if (!dmux->rx) {
> -		bam_dmux_pc_vote(dmux, false);
> -		return -ENXIO;
> +	{
> +		guard(mutex)(&dmux->power_lock);

There is scoped_guard() for this pattern.

> +
> +		/* Ensure that we actually initialized successfully */
> +		if (!dmux->rx) {
> +			ret = -ENXIO;
> +		} else if (!dmux->tx) {
> +			/* Request TX channel if necessary */
> +			dmux->tx = dma_request_chan(dev, "tx");
> +			if (IS_ERR(dmux->tx)) {
> +				dev_err(dev, "Failed to request TX DMA channel: %pe\n",
> +					dmux->tx);
> +				dmux->tx = NULL;
> +				ret = -ENXIO;
> +			}
> +		}
>  	}

Please keep the old structure (if (...) return). If we need to wrap each
of the failure cases with bam_dmux_runtime_suspend() we should perhaps
move the TX initialization into a separate function and then call it
here.

>  
> -	/* Request TX channel if necessary */
> -	if (dmux->tx)
> -		return 0;
> -
> -	dmux->tx = dma_request_chan(dev, "tx");
> -	if (IS_ERR(dmux->tx)) {
> -		dev_err(dev, "Failed to request TX DMA channel: %pe\n", dmux->tx);
> -		dmux->tx = NULL;
> +	if (ret)
>  		bam_dmux_runtime_suspend(dev);
> -		return -ENXIO;
> -	}
>  
> -	return 0;
> +	return ret;
>  }

Thanks,
Stephan