diff mbox series

[1/2] wifi: ath11k: fix null ptr dereference when tx offload is enabled

Message ID 20230308174703.12270-2-quic_pradeepc@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: ath11k: bug fixes in tx offload and stats | expand

Commit Message

Pradeep Kumar Chitrapu March 8, 2023, 5:47 p.m. UTC 
When tx offload is enabled, info->band from skb cb is 0. This
causes null pointer access at mac80211 when sband is accessed.

In offload case, ndo_hard_start will bypass mac80211 tx and no
function will set info->band in skb cb to correct value.

Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1

Signed-off-by: Pradeep Kumar Chitrapu <quic_pradeepc@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/dp_tx.c | 26 ++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

Comments

Felix Fietkau March 8, 2023, 6:09 p.m. UTC | #1 
On 08.03.23 18:47, Pradeep Kumar Chitrapu wrote:
> When tx offload is enabled, info->band from skb cb is 0. This
> causes null pointer access at mac80211 when sband is accessed.
> 
> In offload case, ndo_hard_start will bypass mac80211 tx and no
> function will set info->band in skb cb to correct value.
> 
> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Pradeep Kumar Chitrapu <quic_pradeepc@quicinc.com>
> ---
>   drivers/net/wireless/ath/ath11k/dp_tx.c | 26 ++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c
> index 8afbba236935..0f3a32434970 100644
> --- a/drivers/net/wireless/ath/ath11k/dp_tx.c
> +++ b/drivers/net/wireless/ath/ath11k/dp_tx.c
> @@ -354,8 +364,10 @@ ath11k_dp_tx_htt_tx_complete_buf(struct ath11k_base *ab,
>   			info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
>   		}
>   	}
> -
> -	ieee80211_tx_status(ar->hw, msdu);
> +	if (flags & ATH11K_SKB_HW_80211_ENCAP)
> +		ieee80211_tx_status_8023(ar->hw, vif, msdu);
> +	else
> +		ieee80211_tx_status(ar->hw, msdu);
>   }
>   
>   static void
> @@ -610,7 +627,10 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar,
>   
>   	spin_unlock_bh(&ab->base_lock);
>   
> -	ieee80211_tx_status_ext(ar->hw, &status);
> +	if (flags & ATH11K_SKB_HW_80211_ENCAP)
> +		ieee80211_tx_status_8023(ar->hw, vif, msdu);
> +	else
> +		ieee80211_tx_status_ext(ar->hw, &status);
>   }
>   
>   static inline void ath11k_dp_tx_status_parse(struct ath11k_base *ab,
I think using ieee80211_tx_status_8023 is a bad idea. It is simply a 
wrapper around ieee80211_tx_status_ext which looks up the sta based on 
the MSDU DA. This means it is incompatible with 4-address mode.
If you can have a sta pointer available, it is much better to just use 
ieee80211_tx_status_ext unconditionally.

In fact, I think we should simply remove ieee80211_tx_status_8023.

- Felix
Vasanthakumar Thiagarajan March 9, 2023, 4:02 a.m. UTC | #2 
On 3/8/2023 11:17 PM, Pradeep Kumar Chitrapu wrote:
> When tx offload is enabled, info->band from skb cb is 0. This
> causes null pointer access at mac80211 when sband is accessed.
> 

More specifically tx encap offload instead of tx offload will be clearer.


> In offload case, ndo_hard_start will bypass mac80211 tx and no
> function will set info->band in skb cb to correct value.
> 
> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Pradeep Kumar Chitrapu <quic_pradeepc@quicinc.com>
> ---
>   drivers/net/wireless/ath/ath11k/dp_tx.c | 26 ++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c
> index 8afbba236935..0f3a32434970 100644
> --- a/drivers/net/wireless/ath/ath11k/dp_tx.c
> +++ b/drivers/net/wireless/ath/ath11k/dp_tx.c
> @@ -320,6 +320,8 @@ ath11k_dp_tx_htt_tx_complete_buf(struct ath11k_base *ab,
>   	struct ieee80211_tx_info *info;
>   	struct ath11k_skb_cb *skb_cb;
>   	struct ath11k *ar;
> +	struct ieee80211_vif *vif;
> +	u8 flags = 0;

Is this initialization needed with the way flags is assigned below?

>   
>   	spin_lock(&tx_ring->tx_idr_lock);
>   	msdu = idr_remove(&tx_ring->txbuf_idr, ts->msdu_id);
> @@ -341,6 +343,14 @@ ath11k_dp_tx_htt_tx_complete_buf(struct ath11k_base *ab,
>   
>   	dma_unmap_single(ab->dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
>   
> +	if (!skb_cb->vif) {
> +		dev_kfree_skb_any(msdu);
> +		return;
> +	}
> +
> +	flags = skb_cb->flags;
> +	vif = skb_cb->vif;
> +
>   	memset(&info->status, 0, sizeof(info->status));
>   
>   	if (ts->acked) {
> @@ -354,8 +364,10 @@ ath11k_dp_tx_htt_tx_complete_buf(struct ath11k_base *ab,
>   			info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
>   		}
>   	}
> -
> -	ieee80211_tx_status(ar->hw, msdu);
> +	if (flags & ATH11K_SKB_HW_80211_ENCAP)
> +		ieee80211_tx_status_8023(ar->hw, vif, msdu);
> +	else
> +		ieee80211_tx_status(ar->hw, msdu);
>   }
>   
>   static void
> @@ -524,6 +536,8 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar,
>   	struct ath11k_peer *peer;
>   	struct ath11k_sta *arsta;
>   	struct rate_info rate;
> +	struct ieee80211_vif *vif;
> +	u8 flags = 0;
>   

Same here on the initialization part.

>   	if (WARN_ON_ONCE(ts->buf_rel_source != HAL_WBM_REL_SRC_MODULE_TQM)) {
>   		/* Must not happen */
> @@ -544,6 +558,9 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar,
>   		return;
>   	}
>   
> +	flags = skb_cb->flags;
> +	vif = skb_cb->vif;
> +
>   	info = IEEE80211_SKB_CB(msdu);
>   	memset(&info->status, 0, sizeof(info->status));
>   
> @@ -610,7 +627,10 @@ static void ath11k_dp_tx_complete_msdu(struct ath11k *ar,
>   
>   	spin_unlock_bh(&ab->base_lock);
>   
> -	ieee80211_tx_status_ext(ar->hw, &status);
> +	if (flags & ATH11K_SKB_HW_80211_ENCAP)
> +		ieee80211_tx_status_8023(ar->hw, vif, msdu);
> +	else
> +		ieee80211_tx_status_ext(ar->hw, &status);
>   }
>   
>   static inline void ath11k_dp_tx_status_parse(struct ath11k_base *ab,


Vasanth
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c b/drivers/net/wireless/ath/ath11k/dp_tx.c
index 8afbba236935..0f3a32434970 100644
--- a/drivers/net/wireless/ath/ath11k/dp_tx.c
+++ b/drivers/net/wireless/ath/ath11k/dp_tx.c
@@ -320,6 +320,8 @@  ath11k_dp_tx_htt_tx_complete_buf(struct ath11k_base *ab,
 	struct ieee80211_tx_info *info;
 	struct ath11k_skb_cb *skb_cb;
 	struct ath11k *ar;
+	struct ieee80211_vif *vif;
+	u8 flags = 0;
 
 	spin_lock(&tx_ring->tx_idr_lock);
 	msdu = idr_remove(&tx_ring->txbuf_idr, ts->msdu_id);
@@ -341,6 +343,14 @@  ath11k_dp_tx_htt_tx_complete_buf(struct ath11k_base *ab,
 
 	dma_unmap_single(ab->dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
 
+	if (!skb_cb->vif) {
+		dev_kfree_skb_any(msdu);
+		return;
+	}
+
+	flags = skb_cb->flags;
+	vif = skb_cb->vif;
+
 	memset(&info->status, 0, sizeof(info->status));
 
 	if (ts->acked) {
@@ -354,8 +364,10 @@  ath11k_dp_tx_htt_tx_complete_buf(struct ath11k_base *ab,
 			info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
 		}
 	}
-
-	ieee80211_tx_status(ar->hw, msdu);
+	if (flags & ATH11K_SKB_HW_80211_ENCAP)
+		ieee80211_tx_status_8023(ar->hw, vif, msdu);
+	else
+		ieee80211_tx_status(ar->hw, msdu);
 }
 
 static void
@@ -524,6 +536,8 @@  static void ath11k_dp_tx_complete_msdu(struct ath11k *ar,
 	struct ath11k_peer *peer;
 	struct ath11k_sta *arsta;
 	struct rate_info rate;
+	struct ieee80211_vif *vif;
+	u8 flags = 0;
 
 	if (WARN_ON_ONCE(ts->buf_rel_source != HAL_WBM_REL_SRC_MODULE_TQM)) {
 		/* Must not happen */
@@ -544,6 +558,9 @@  static void ath11k_dp_tx_complete_msdu(struct ath11k *ar,
 		return;
 	}
 
+	flags = skb_cb->flags;
+	vif = skb_cb->vif;
+
 	info = IEEE80211_SKB_CB(msdu);
 	memset(&info->status, 0, sizeof(info->status));
 
@@ -610,7 +627,10 @@  static void ath11k_dp_tx_complete_msdu(struct ath11k *ar,
 
 	spin_unlock_bh(&ab->base_lock);
 
-	ieee80211_tx_status_ext(ar->hw, &status);
+	if (flags & ATH11K_SKB_HW_80211_ENCAP)
+		ieee80211_tx_status_8023(ar->hw, vif, msdu);
+	else
+		ieee80211_tx_status_ext(ar->hw, &status);
 }
 
 static inline void ath11k_dp_tx_status_parse(struct ath11k_base *ab,