Re: [PATCH ath-next v2 08/17] wifi: ath12k: Add hif operation support for QCN6432

[Aaradhana Sahu <aaradhana.sahu@oss.qualcomm.com>]

On 10/16/2026 9:16 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] net: ath12k: unconfigured dynamic window in pcic read32/write32 causes out-of-bounds MMIO
> - [High] wifi: ath12k: use-after-free in timer callback ath12k_ce_rx_replenish_retry()
> --
> 
> commit d724e1834e7dbd2c47e113791169e389fe945179
> Author: Sowmiya Sree Elavalagan <quic_ssreeela@quicinc.com>
> wifi: ath12k: Add hif operation support for QCN6432
> 
> This commit adds HIF operations for the QCN6432 Wi-Fi driver by reusing
> existing methods from IPQ5332 and QCN9274 where applicable. It implements
> QCN6432-specific operations in a new pcic.c file.
> 
>> diff --git a/drivers/net/wireless/ath/ath12k/pcic.c b/drivers/net/wireless/ath/ath12k/pcic.c
>> new file mode 100644
>> index 0000000000000..e3c8476af459c
>> --- /dev/null
>> +++ b/drivers/net/wireless/ath/ath12k/pcic.c
> 
> [ ... ]
> 
>> +void ath12k_pcic_stop(struct ath12k_base *ab)
>> +{
>> +	ath12k_pci_ce_irq_disable_sync(ab);
>> +	timer_delete_sync(&ab->rx_replenish_retry);
>> +	ath12k_ce_cleanup_pipes(ab);
>> +}
> 
> [Severity: High]
> Is there a mechanism preventing ab->rx_replenish_retry from re-arming itself
> here?
> 
> If ath12k_pcic_stop() executes while the timer callback
> ath12k_ce_rx_replenish_retry() is running concurrently, timer_delete_sync()
> will wait. However, the callback invokes ath12k_ce_rx_post_buf(), which blindly
> re-arms the timer on allocation failures (-ENOMEM):
> 
> drivers/net/wireless/ath/ath12k/ce.c:ath12k_ce_rx_post_buf() {
>     ...
>     ret = ath12k_ce_rx_post_pipe(pipe);
>     if (ret) {
>         if (ret == -ENOSPC)
>             continue;
>         ...
>         mod_timer(&ab->rx_replenish_retry, ...);
>         return;
>     }
> }
> 
> Could this cause the timer to be re-enqueued after timer_delete_sync()
> unblocks, leading to a use-after-free regression when the timer fires after the
> structures are freed?
> 

The CE IRQs are disabled and synchronized before timer_delete_sync() is called.
This ensures that no new CE IRQ processing can trigger RX replenishment during
shutdown. timer_delete_sync() also waits for any currently running timer callback
to complete before the CE pipes are cleaned up.

> [ ... ]
> 
>> +u32 ath12k_pcic_read32(struct ath12k_base *ab, u32 offset)
>> +{
>> +	u32 window_start;
>> +
>> +	if (offset < WINDOW_START)
>> +		return readl_relaxed(ab->mem + offset);
>> +
>> +	window_start = ath12k_pcic_get_window_start(ab, offset);
>> +	return readl_relaxed(ab->mem + window_start + (offset & WINDOW_RANGE_MASK));
>> +}
> 
> [Severity: Critical]
> Does this read from an unconfigured dynamic window?
> 
> When offset does not fall into the statically mapped UMAC or CE ranges,
> ath12k_pcic_get_window_start() returns the fallback dynamic window address
> (WINDOW_START, which is Window 1).
> 
> However, unlike the cmem accessors or the PCI equivalent (ath12k_pci_read32)
> which explicitly lock and program Window 1 to the high bits of the target
> offset, this function skips configuration entirely and blindly performs the
> read.
> 
> Could this result in out-of-bounds MMIO reads, directing the read to whatever
> physical memory address the dynamic window was previously configured to map?
> 

This should not happen for QCN6432, as it uses the CMEM read/write operations for
accesses to windows that are not statically mapped. These accesses do not use this
function.

>> +
>> +void ath12k_pcic_write32(struct ath12k_base *ab, u32 offset, u32 value)
>> +{
>> +	u32 window_start;
>> +
>> +	if (offset < WINDOW_START) {
>> +		writel_relaxed(value, ab->mem + offset);
>> +		return;
>> +	}
>> +
>> +	window_start = ath12k_pcic_get_window_start(ab, offset);
>> +	writel_relaxed(value, ab->mem + window_start + (offset & WINDOW_RANGE_MASK));
>> +}
> 
> [Severity: Critical]
> Similarly, does this write to the dynamic window without first programming the
> hardware register to map the target offset?
> 
> Like the read path above, ath12k_pcic_write32() uses the dynamic window without
> calling a window selection function such as ath12k_pcic_select_static_window().
> 
> Will this regression corrupt memory or hardware state by writing to a previously
> configured but currently unrelated memory address?
> 

same explanation as above.