diff mbox series

[v5] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Message ID 20241127143804.30075-1-zghbqbc@gmail.com
State New
Headers show
Series [v5] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe() | expand

Commit Message

Baichuan Qi Nov. 27, 2024, 2:38 p.m. UTC
Current implementation of `ath11k_ce_rx_post_pipe()` checks for
NON-NULL of either `dest_ring` or `status_ring` using an OR (||).
Both rings, especially `dest_ring`, should be ensured to be
NON-NULL in this function.

If only one of the rings is valid, such as `dest_ring` is NULL
and `status_ring` is NON-NULL, the OR (||) check would not stop
`ath11k_ce_rx_post_pipe()`, the subsequent call to
`ath11k_ce_rx_buf_enqueue_pipe()` will access the NULL pointer,
resulting in a driver crash.

Fix the NON-NULL check by changing the OR (||) to AND (&&),
and return an error code `-EIO` to indicate
`ath11k_ce_rx_post_pipe()` is stopped with an NULL pointer 
error, ensuring that the function only proceeds when both 
`dest_ring` and `status_ring` are NON-NULL.

Link: https://lore.kernel.org/ath11k/a9ccc947-20b2-4322-84e5-c96aaa604e63@web.de
Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
---
V4 -> V5: add err code in NULL check
V3 -> V4: reorder describe info
V2 -> V3: add Link URL to mailing list archives
V1 -> V2: rewrite commit message and fix tag

 drivers/net/wireless/ath/ath11k/ce.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Vasanthakumar Thiagarajan Nov. 27, 2024, 3:23 p.m. UTC | #1
On 11/27/2024 8:08 PM, Baichuan Qi wrote:
> Current implementation of `ath11k_ce_rx_post_pipe()` checks for
> NON-NULL of either `dest_ring` or `status_ring` using an OR (||).
> Both rings, especially `dest_ring`, should be ensured to be
> NON-NULL in this function.
> 
> If only one of the rings is valid, such as `dest_ring` is NULL
> and `status_ring` is NON-NULL, the OR (||) check would not stop
> `ath11k_ce_rx_post_pipe()`, the subsequent call to
> `ath11k_ce_rx_buf_enqueue_pipe()` will access the NULL pointer,
> resulting in a driver crash.
> 
> Fix the NON-NULL check by changing the OR (||) to AND (&&),
> and return an error code `-EIO` to indicate
> `ath11k_ce_rx_post_pipe()` is stopped with an NULL pointer
> error, ensuring that the function only proceeds when both
> `dest_ring` and `status_ring` are NON-NULL.
> 
> Link: https://lore.kernel.org/ath11k/a9ccc947-20b2-4322-84e5-c96aaa604e63@web.de
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")

This does not really fix any real issue. Please check ath11k_ce_alloc_pipe()
where initialization would fail if anyone of pipe->dest_ring and
pipe->status_ring allocation fails for ce pipe used for Rx.

> Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
> ---
> V4 -> V5: add err code in NULL check
> V3 -> V4: reorder describe info
> V2 -> V3: add Link URL to mailing list archives
> V1 -> V2: rewrite commit message and fix tag
> 
>   drivers/net/wireless/ath/ath11k/ce.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
> index e66e86bdec20..223dab928453 100644
> --- a/drivers/net/wireless/ath/ath11k/ce.c
> +++ b/drivers/net/wireless/ath/ath11k/ce.c
> @@ -324,8 +324,10 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
>   	dma_addr_t paddr;
>   	int ret = 0;
>   
> -	if (!(pipe->dest_ring || pipe->status_ring))
> -		return 0;
> +	if (!(pipe->dest_ring && pipe->status_ring)) {
> +		ret = -EIO;
> +		return ret;
> +	}

This will always fail as the caller loops through all the supported ce pipes
and ce pipes used for Tx will not have either dest_ring or status_ring.
Please ensure the patch is tested properly.

So NAK

Vasanth
Baichuan Qi Nov. 28, 2024, 6:09 a.m. UTC | #2
Thanks for your reply

The reason I submit this patch is that the current 
`ath11k_ce_rx_post_pipe()` NULL pointer check does not ensure 
that `dest_ring` is NON-NULL. And it is not clear to show the 
filtering of tx ce pipes

> This does not really fix any real issue. Please check ath11k_ce_alloc_pipe()
> where initialization would fail if anyone of pipe->dest_ring and
> pipe->status_ring allocation fails for ce pipe used for Rx.

When the driver is running normally, the results of the 
following three are equal:
---
(pipe->dest_ring || pipe->status_ring) // current code
(pipe->dest_ring && pipe->status_ring)
(pipe->dest_ring)
---
However, when some errors occur and `dest_ring` is abnormal,
the OR operation cannot guarantee that the pointer is NON-NULL.

> This will always fail as the caller loops through all the supported ce pipes
> and ce pipes used for Tx will not have either dest_ring or status_ring.
> Please ensure the patch is tested properly.

I tested [PATCH v5] and indeed the wrong return value will lead 
to wrong results when the pointer is null.

Please refer to [PATCH v4].
Link: https://lore.kernel.org/ath11k/20241127114310.26085-1-zghbqbc@gmail.com/
Although it does not return an error code, it can ensure that 
when an unknown error occurs and causes the status of 
`dest_ring` and `status_ring` to be different, the subsequent 
code will not access the null pointer, which will only 
cause the driver to fall into loops.

Thanks for you read.

Thanks
Baichuan Qi
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index e66e86bdec20..223dab928453 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -324,8 +324,10 @@  static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
 	dma_addr_t paddr;
 	int ret = 0;
 
-	if (!(pipe->dest_ring || pipe->status_ring))
-		return 0;
+	if (!(pipe->dest_ring && pipe->status_ring)) {
+		ret = -EIO;
+		return ret;
+	}
 
 	spin_lock_bh(&ab->ce.ce_lock);
 	while (pipe->rx_buf_needed) {