Re: [PATCH] ath10k: rebuild crypto header in RX data frames

[Jasmine Strong]

That can't be the case here, since we see it break the mesh too (where
the clients are other QCA radios.)

We're now seeing very slow mesh peering with a 9882 leaf to a 4019
gateway, with the second version of this patch.
It took more than five minutes for one of the leaves to successfully
peer (and the other 9882 leaf, despite achieving
peering, never managed to get DHCP, implying that something is broken
with data frames and this patch on qca9882.)

On Tue, Oct 24, 2017 at 9:09 AM, Ben Greear  wrote:
> On 10/23/2017 09:50 PM, Kalle Valo wrote:
>>
>> Jasmine Strong  writes:
>>
>>> That's what I saw.  A bcom client was able to associate and not pass
>>> any traffic.  This is on all three of 9882, 9888 and 4019.
>>
>>
>> Thanks for the report, we'll investigate it. And I see that your email
>> was now succesfully delivered to the list:
>>
>> http://lists.infradead.org/pipermail/ath10k/2017-October/010325.html
>
>
>
> I'm not sure if this is helpful or not, but in the transition from 10.1 to
> 10.2 firmware,
> there was some tricky re-work of the PN counter logic in the firmware
> source.  It had
> specific impact on the broadcom driver interaction.
>
> Possibly a similar issue is being seen with this driver patch?
>
> Thanks,
> Ben
>
> --
> Ben Greear 
> Candela Technologies Inc  http://www.candelatech.com
>

Re: [PATCH] ath10k: rebuild crypto header in RX data frames

[Ben Greear]

On 10/23/2017 09:50 PM, Kalle Valo wrote:

Jasmine Strong  writes:


That's what I saw.  A bcom client was able to associate and not pass
any traffic.  This is on all three of 9882, 9888 and 4019.


Thanks for the report, we'll investigate it. And I see that your email
was now succesfully delivered to the list:

http://lists.infradead.org/pipermail/ath10k/2017-October/010325.html



I'm not sure if this is helpful or not, but in the transition from 10.1 to 10.2 
firmware,
there was some tricky re-work of the PN counter logic in the firmware source.  
It had
specific impact on the broadcom driver interaction.

Possibly a similar issue is being seen with this driver patch?

Thanks,
Ben

--
Ben Greear 
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath10k: rebuild crypto header in RX data frames

[Kalle Valo]

Jasmine Strong  writes:

> That's what I saw.  A bcom client was able to associate and not pass
> any traffic.  This is on all three of 9882, 9888 and 4019.

Thanks for the report, we'll investigate it. And I see that your email
was now succesfully delivered to the list:

http://lists.infradead.org/pipermail/ath10k/2017-October/010325.html

-- 
Kalle Valo

Re: [PATCH] ath10k: rebuild crypto header in RX data frames

[Sebastian Gottschall]

Am 23.10.2017 um 16:24 schrieb Vasanthakumar Thiagarajan:

On Saturday 21 October 2017 01:41 AM, Sebastian Gottschall wrote:

i suggest the following patch on top of yours. please tell me if my thoughts 
are correct here. its mainly a guess

I agree we need to take care of this for newly added ciphers as well. How about 
making it as a separate patch
to make the patch bit smaller and to reduce the difficulties in back porting it 
for ciphers supported for long
time?
the patch for these both ciphers doesnt make it bigger. just a few 
lines. the main problem right now is that the original patch without my 
enhancements isnt working. it breaks encryption

Vasanth



--
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565

Re: [PATCH] ath10k: rebuild crypto header in RX data frames

[Vasanthakumar Thiagarajan]

On Saturday 21 October 2017 01:41 AM, Sebastian Gottschall wrote:
> i suggest the following patch on top of yours. please tell me if my thoughts 
> are correct here. its mainly a guess

I agree we need to take care of this for newly added ciphers as well. How about 
making it as a separate patch 
to make the patch bit smaller and to reduce the difficulties in back porting it 
for ciphers supported for long 
time?

Vasanth

Re: [PATCH] ath10k: rebuild crypto header in RX data frames

[Kalle Valo]

Jasmine Strong  writes:

> When we tried this patch, it completely broke all wpa2-ccmp-aes
> traffic.

Which patch, Vasanth's or Sebastian's? I even tested myself, with both
CCMP and TKIP on both AP and client modes, and didn't see see any
problems. What kind of setup you have?

I tested on a x86 laptop and current ath.git master branch:

ath10k_pci :02:00.0: pci irq msi oper_irq_mode 2 irq_mode 0 reset_mode 0
ath10k_pci :02:00.0: qca988x hw2.0 target 0x4100016c chip_id 0x043202ff sub 
:
ath10k_pci :02:00.0: kconfig debug 1 debugfs 1 tracing 1 dfs 1 testmode 1
ath10k_pci :02:00.0: firmware ver 10.2.4.70.66 api 5 features 
no-p2p,raw-mode,mfp,allows-mesh-bcast crc32 c2dd2ad5
ath10k_pci :02:00.0: board_file api 1 bmi_id N/A crc32 bebc7c08

And my hostapd.conf:

driver=nl80211
hw_mode=a
channel=36
ieee80211n=1
interface=wlan0
ctrl_interface=/var/run/hostapd
ctrl_interface_group=adm
ssid=test-psk
wpa=2
wpa_key_mgmt=WPA-PSK
wpa_pairwise=CCMP
wpa_passphrase=12345678


-- 
Kalle Valo

Re: [PATCH] ath10k: rebuild crypto header in RX data frames

[Sebastian Gottschall]

i suggest the following patch on top of yours. please tell me if my 
thoughts are correct here. its mainly a guess


--- htt_rx.c    (revision 3656)
+++ htt_rx.c    (working copy)
@@ -550,6 +550,11 @@
    return IEEE80211_TKIP_IV_LEN;
    case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
    return IEEE80211_CCMP_HDR_LEN;
+   case HTT_RX_MPDU_ENCRYPT_AES_CCMP_256:
+   return IEEE80211_CCMP_256_HDR_LEN;
+   case HTT_RX_MPDU_ENCRYPT_AES_GCMP_128:
+   case HTT_RX_MPDU_ENCRYPT_AES_GCMP_256:
+   return IEEE80211_GCMP_HDR_LEN;
    case HTT_RX_MPDU_ENCRYPT_WEP128:
    case HTT_RX_MPDU_ENCRYPT_WAPI:
    break;
@@ -575,6 +580,11 @@
    return IEEE80211_TKIP_ICV_LEN;
    case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
    return IEEE80211_CCMP_MIC_LEN;
+   case HTT_RX_MPDU_ENCRYPT_AES_CCMP_256:
+   return IEEE80211_CCMP_256_MIC_LEN;
+   case HTT_RX_MPDU_ENCRYPT_AES_GCMP_128:
+   case HTT_RX_MPDU_ENCRYPT_AES_GCMP_256:
+   return IEEE80211_GCMP_MIC_LEN;
    case HTT_RX_MPDU_ENCRYPT_WEP128:
    case HTT_RX_MPDU_ENCRYPT_WAPI:
    break;
@@ -1012,6 +1022,7 @@
    return;
    case HTT_RX_MPDU_ENCRYPT_WEP40:
    case HTT_RX_MPDU_ENCRYPT_WEP104:
+   case HTT_RX_MPDU_ENCRYPT_WEP128:
    hdr = skb_push(msdu, IEEE80211_WEP_IV_LEN);
    memcpy(hdr, rxd->mpdu_start.pn, IEEE80211_WEP_IV_LEN - 1);
    hdr[3] = rxd->msdu_end.common.key_id_octet;
@@ -1032,7 +1043,21 @@
    hdr[3] = 0x20 | (rxd->msdu_end.common.key_id_octet << 6);
    memcpy(hdr + 4, rxd->mpdu_start.pn + 2, 4);
    return;
-   case HTT_RX_MPDU_ENCRYPT_WEP128:
+   case HTT_RX_MPDU_ENCRYPT_AES_CCMP_256:
+   hdr = skb_push(msdu, IEEE80211_CCMP_256_HDR_LEN);
+   memcpy(hdr, rxd->mpdu_start.pn, 2);
+   hdr[2] = 0;
+   hdr[3] = 0x20 | (rxd->msdu_end.common.key_id_octet << 6);
+   memcpy(hdr + 4, rxd->mpdu_start.pn + 2, 4);
+   return;
+   case HTT_RX_MPDU_ENCRYPT_AES_GCMP_128:
+   case HTT_RX_MPDU_ENCRYPT_AES_GCMP_256:
+   hdr = skb_push(msdu, IEEE80211_GCMP_HDR_LEN);
+   memcpy(hdr, rxd->mpdu_start.pn, 2);
+   hdr[2] = 0;
+   hdr[3] = 0x20 | (rxd->msdu_end.common.key_id_octet << 6);
+   memcpy(hdr + 4, rxd->mpdu_start.pn + 2, 4);
+   return;
    case HTT_RX_MPDU_ENCRYPT_WAPI:
    return;
    default:
@@ -1098,16 +1123,41 @@
    hdr = (void *)msdu->data;

    /* MIC */
-   if ((status->flag & RX_FLAG_MIC_STRIPPED) &&
-   enctype == HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2)
-   skb_trim(msdu, msdu->len - 8);
-
+   if (status->flag & RX_FLAG_MIC_STRIPPED) {
+   switch(enctype)
+   {
+   case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
+   skb_trim(msdu, msdu->len - IEEE80211_CCMP_MIC_LEN);
+   break;
+   case HTT_RX_MPDU_ENCRYPT_AES_CCMP_256:
+   skb_trim(msdu, msdu->len - IEEE80211_CCMP_256_MIC_LEN);
+   break;
+   case HTT_RX_MPDU_ENCRYPT_AES_GCMP_128:
+   skb_trim(msdu, msdu->len - IEEE80211_GCMP_MIC_LEN);
+   break;
+   case HTT_RX_MPDU_ENCRYPT_AES_GCMP_256:
+   skb_trim(msdu, msdu->len - IEEE80211_GCMP_MIC_LEN);
+   break;
+   default:
+   break;
+   }
+   }
    /* ICV */
-   if (status->flag & RX_FLAG_ICV_STRIPPED &&
-   enctype != HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2)
+   if (status->flag & RX_FLAG_ICV_STRIPPED) {
+   switch(enctype)
+   {
+   case HTT_RX_MPDU_ENCRYPT_WEP40:
+   case HTT_RX_MPDU_ENCRYPT_WEP104:
+   case HTT_RX_MPDU_ENCRYPT_TKIP_WITHOUT_MIC:
+   case HTT_RX_MPDU_ENCRYPT_WEP128:
+   case HTT_RX_MPDU_ENCRYPT_TKIP_WPA:
    skb_trim(msdu, msdu->len -
 ath10k_htt_rx_crypto_tail_len(ar, enctype));
-
+   break;
+   default:
+   break;
+   }
+   }
    /* MMIC */
    if ((status->flag & RX_FLAG_MMIC_STRIPPED) &&
    !ieee80211_has_morefrags(hdr->frame_control) &&
Index: rx_desc.h
===
--- rx_desc.h   (revision 3656)
+++ rx_desc.h   (working copy)
@@ -239,6 +239,9 @@
    HTT_RX_MPDU_ENCRYPT_WAPI = 5,
    HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2 = 6,
    HTT_RX_MPDU_ENCRYPT_NONE = 7,
+   HTT_RX_MPDU_ENCRYPT_AES_CCMP_256 = 8,
+   HTT_RX_MPDU_ENCRYPT_AES_GCMP_128 = 9,
+   HTT_RX_MPDU_ENCRYPT_AES_GCMP_256 = 10,
 };

 #define RX_MPDU_START_INFO0_PEER_IDX_MASK 0x07ff



Am 20.10.2017 um 18:28 schrieb Kalle Valo:

From: Vasanthakumar Thiagarajan 

RX data frames notified through 

Re: [PATCH] ath10k: rebuild crypto header in RX data frames

[Sebastian Gottschall]

maybe this small patch hint here should help to make this patch better

--- rx_desc.h   (revision 3655)
+++ rx_desc.h   (working copy)
@@ -239,6 +239,9 @@
    HTT_RX_MPDU_ENCRYPT_WAPI = 5,
    HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2 = 6,
    HTT_RX_MPDU_ENCRYPT_NONE = 7,
+   HTT_RX_MPDU_ENCRYPT_AES_CCMP_256 = 8,
+   HTT_RX_MPDU_ENCRYPT_AES_GCMP_128 = 9,
+   HTT_RX_MPDU_ENCRYPT_AES_GCMP_256 = 10,

Am 20.10.2017 um 18:28 schrieb Kalle Valo:

From: Vasanthakumar Thiagarajan 

RX data frames notified through HTT_T2H_MSG_TYPE_RX_IND and
HTT_T2H_MSG_TYPE_RX_FRAG_IND expect PN/TSC check to be done
on host (mac80211) rather than firmware. Rebuild cipher header
in every received data frames (that are notified through those
HTT interfaces) from the PN/TSC and key_id information available
from rx descriptor of the first msdu of each mpdu. Skip setting
RX_FLAG_IV_STRIPPED flag for the packets which requires mac80211
PN/TSC check support and set appropriate RX_FLAG for stripped
crypto tail. QCA988X, QCA9887, QCA99X0, QCA9984, QCA9888 and
QCA4019 currently need the rebuilding of cipher header to perform
PN/TSC check for replay attack.

Signed-off-by: Vasanthakumar Thiagarajan 
Signed-off-by: Kalle Valo 
---
  drivers/net/wireless/ath/ath10k/htt_rx.c |  120 ++
  1 file changed, 104 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c 
b/drivers/net/wireless/ath/ath10k/htt_rx.c
index a3f5dc78353f..9a070ad05179 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -995,8 +995,55 @@ static int ath10k_htt_rx_nwifi_hdrlen(struct ath10k *ar,
return len;
  }
  
+static void ath10k_htt_rx_build_crypto_hdr(struct ath10k *ar,

+  struct sk_buff *msdu,
+  struct htt_rx_desc *rxd,
+  struct ieee80211_rx_status *status,
+  enum htt_rx_mpdu_encrypt_type type)
+{
+   u8 *hdr;
+
+   if (!(status->flag & RX_FLAG_DECRYPTED) ||
+   status->flag & RX_FLAG_IV_STRIPPED)
+   return;
+
+   switch (type) {
+   case HTT_RX_MPDU_ENCRYPT_NONE:
+   return;
+   case HTT_RX_MPDU_ENCRYPT_WEP40:
+   case HTT_RX_MPDU_ENCRYPT_WEP104:
+   hdr = skb_push(msdu, IEEE80211_WEP_IV_LEN);
+   memcpy(hdr, rxd->mpdu_start.pn, IEEE80211_WEP_IV_LEN - 1);
+   hdr[3] = rxd->msdu_end.common.key_id_octet;
+   return;
+   case HTT_RX_MPDU_ENCRYPT_TKIP_WITHOUT_MIC:
+   case HTT_RX_MPDU_ENCRYPT_TKIP_WPA:
+   hdr = skb_push(msdu, IEEE80211_TKIP_IV_LEN);
+   hdr[0] = rxd->mpdu_start.pn[1];
+   hdr[1] = 0;
+   hdr[2] = rxd->mpdu_start.pn[0];
+   hdr[3] = 0x20 | (rxd->msdu_end.common.key_id_octet << 6);
+   memcpy(hdr + 4, rxd->mpdu_start.pn + 2, 4);
+   return;
+   case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
+   hdr = skb_push(msdu, IEEE80211_CCMP_HDR_LEN);
+   memcpy(hdr, rxd->mpdu_start.pn, 2);
+   hdr[2] = 0;
+   hdr[3] = 0x20 | (rxd->msdu_end.common.key_id_octet << 6);
+   memcpy(hdr + 4, rxd->mpdu_start.pn + 2, 4);
+   return;
+   case HTT_RX_MPDU_ENCRYPT_WEP128:
+   case HTT_RX_MPDU_ENCRYPT_WAPI:
+   return;
+   default:
+   ath10k_warn(ar, "unsupported encryption type %d\n", type);
+   return;
+   }
+}
+
  static void ath10k_htt_rx_h_undecap_raw(struct ath10k *ar,
struct sk_buff *msdu,
+   struct htt_rx_desc *first_rxd,
struct ieee80211_rx_status *status,
enum htt_rx_mpdu_encrypt_type enctype,
bool is_decrypted)
@@ -1050,8 +1097,14 @@ static void ath10k_htt_rx_h_undecap_raw(struct ath10k 
*ar,
  
  	hdr = (void *)msdu->data;
  
-	/* Tail */

-   if (status->flag & RX_FLAG_IV_STRIPPED)
+   /* MIC */
+   if ((status->flag & RX_FLAG_MIC_STRIPPED) &&
+   enctype == HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2)
+   skb_trim(msdu, msdu->len - 8);
+
+   /* ICV */
+   if (status->flag & RX_FLAG_ICV_STRIPPED &&
+   enctype != HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2)
skb_trim(msdu, msdu->len -
 ath10k_htt_rx_crypto_tail_len(ar, enctype));
  
@@ -1075,7 +1128,9 @@ static void ath10k_htt_rx_h_undecap_raw(struct ath10k *ar,

  static void ath10k_htt_rx_h_undecap_nwifi(struct ath10k *ar,
  struct sk_buff *msdu,
  struct