Re: [PATCH ath-next 1/3] wifi: ath12k: validate RX frame length before undecapsulation

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]

> 
> 
> On 10/16/2026 11:16 PM, Lorenzo Bianconi wrote:
> > Reject truncated raw frames before removing the FCS trailer. Without this
> > check, subtracting FCS_LEN from a shorter skb underflows and leaves the
> 
> skb_trim() has the check itself, so there will be no underflow?

I agree the commit message is misleading, I think we just need to return an
error in case the skb does not contain the FCS.

> 
> > FCS untrimmed.
> > 
> > Also verify that the frame contains enough data for the 802.11 header and
> > the crypto header before moving the header to make room for the crypto
> > parameters.
> > 
> > Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.1.c7-00108-QCAHMTSWPL_V1.0_V2.0_SILICONZ_UPSTREAM-3
> > 
> > Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> > ---
> >  drivers/net/wireless/ath/ath12k/dp_rx.c       | 46 ++++++++++++++++++---------
> >  drivers/net/wireless/ath/ath12k/dp_rx.h       | 10 +++---
> >  drivers/net/wireless/ath/ath12k/wifi7/dp_rx.c | 22 +++++++++----
> >  3 files changed, 51 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
> > index 36be543b7c50..40b8da34268f 100644
> > --- a/drivers/net/wireless/ath/ath12k/dp_rx.c
> > +++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
> > @@ -999,11 +999,11 @@ static void ath12k_dp_rx_h_undecap_nwifi(struct ath12k_pdev_dp *dp_pdev,
> >  	memcpy(skb_push(msdu, hdr_len), decap_hdr, hdr_len);
> >  }
> >  
> > -static void ath12k_dp_rx_h_undecap_raw(struct ath12k_pdev_dp *dp_pdev,
> > -				       struct sk_buff *msdu,
> > -				       enum hal_encrypt_type enctype,
> > -				       struct ieee80211_rx_status *status,
> > -				       bool decrypted)
> > +static int ath12k_dp_rx_h_undecap_raw(struct ath12k_pdev_dp *dp_pdev,
> > +				      struct sk_buff *msdu,
> > +				      enum hal_encrypt_type enctype,
> > +				      struct ieee80211_rx_status *status,
> > +				      bool decrypted)
> >  {
> >  	struct ath12k_dp *dp = dp_pdev->dp;
> >  	struct ath12k_skb_rxcb *rxcb = ATH12K_SKB_RXCB(msdu);
> > @@ -1014,13 +1014,16 @@ static void ath12k_dp_rx_h_undecap_raw(struct ath12k_pdev_dp *dp_pdev,
> >  	if (!rxcb->is_first_msdu ||
> >  	    !(rxcb->is_first_msdu && rxcb->is_last_msdu)) {
> >  		WARN_ON_ONCE(1);
> > -		return;
> > +		return 0;
> >  	}
> >  
> > +	if (msdu->len < FCS_LEN)
> > +		return -EINVAL;
> > +
> 
> every later trim in this function has the same problem: msdu->len minus the MIC, ICV or
> MMIC length is not checked either. should we add check for them as well?

ack, I will fix it in v2.

Regards,
Lorenzo

> 
> >  	skb_trim(msdu, msdu->len - FCS_LEN);
> >  
> >  	if (!decrypted)
> > -		return;
> > +		return 0;
> >  
> >  	hdr = (void *)msdu->data;
> >  
> 
> [...]
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]