2017-10-26 16:36:29

by Kalle Valo

[permalink] [raw]
Subject: [PATCH v3] ath10k: rebuild crypto header in rx data frames

From: Vasanthakumar Thiagarajan <[email protected]>

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 rx_hdr_status tlv available in the
rx descriptor of the first msdu. 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. Hw QCA988X,
QCA9887, QCA99X0, QCA9984, QCA9888 and QCA4019 currently need the
rebuilding of cipher header to perform PN/TSC check for replay
attack.

Please note that removing crypto tail for CCMP-256, GCMP and GCMP-256 ciphers
in raw mode needs to be fixed. Since Rx with these ciphers in raw
mode does not work in the current form even without this patch and
removing crypto tail for these chipers needs clean up, raw mode related
issues in CCMP-256, GCMP and GCMP-256 can be addressed in follow up
patches.

Tested-by: Manikanta Pubbisetty <[email protected]>
Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
Signed-off-by: Kalle Valo <[email protected]>
---

v3:

* Fix cipher header construction for CCMP-256, GCMP and GCMP-256
ciphers.

* Fix bug in raw mode when RX_FLAG_IV_STRIPPED is set.

v2:

* Construct cipher header from rx_hdr_status tlv rather than from
the msdu_start and mpdu_start tlvs. This fixes connection
issues, also reduces the amount of code change.

* Make sure the frame is qos data before clearing AMSDU_PRESENT
bit of qos control field.

* Fix traffic issue with QCA988X and QCA9887 hw by taking care of
padding bytes added for 4-byte alignment before the cipher
header.

drivers/net/wireless/ath/ath10k/htt_rx.c | 105 +++++++++++++++++++++++++-----
drivers/net/wireless/ath/ath10k/rx_desc.h | 3 +
2 files changed, 92 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..5beb6ee0f091 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -550,6 +550,11 @@ static int ath10k_htt_rx_crypto_param_len(struct ath10k *ar,
return IEEE80211_TKIP_IV_LEN;
case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
return IEEE80211_CCMP_HDR_LEN;
+ case HTT_RX_MPDU_ENCRYPT_AES_CCM256_WPA2:
+ return IEEE80211_CCMP_256_HDR_LEN;
+ case HTT_RX_MPDU_ENCRYPT_AES_GCMP_WPA2:
+ case HTT_RX_MPDU_ENCRYPT_AES_GCMP256_WPA2:
+ return IEEE80211_GCMP_HDR_LEN;
case HTT_RX_MPDU_ENCRYPT_WEP128:
case HTT_RX_MPDU_ENCRYPT_WAPI:
break;
@@ -575,6 +580,11 @@ static int ath10k_htt_rx_crypto_tail_len(struct ath10k *ar,
return IEEE80211_TKIP_ICV_LEN;
case HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2:
return IEEE80211_CCMP_MIC_LEN;
+ case HTT_RX_MPDU_ENCRYPT_AES_CCM256_WPA2:
+ return IEEE80211_CCMP_256_MIC_LEN;
+ case HTT_RX_MPDU_ENCRYPT_AES_GCMP_WPA2:
+ case HTT_RX_MPDU_ENCRYPT_AES_GCMP256_WPA2:
+ return IEEE80211_GCMP_MIC_LEN;
case HTT_RX_MPDU_ENCRYPT_WEP128:
case HTT_RX_MPDU_ENCRYPT_WAPI:
break;
@@ -1051,9 +1061,21 @@ static void ath10k_htt_rx_h_undecap_raw(struct ath10k *ar,
hdr = (void *)msdu->data;

/* Tail */
- if (status->flag & RX_FLAG_IV_STRIPPED)
+ if (status->flag & RX_FLAG_IV_STRIPPED) {
skb_trim(msdu, msdu->len -
ath10k_htt_rx_crypto_tail_len(ar, enctype));
+ } else {
+ /* 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));
+ }

/* MMIC */
if ((status->flag & RX_FLAG_MMIC_STRIPPED) &&
@@ -1075,7 +1097,8 @@ 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 ieee80211_rx_status *status,
- const u8 first_hdr[64])
+ const u8 first_hdr[64],
+ enum htt_rx_mpdu_encrypt_type enctype)
{
struct ieee80211_hdr *hdr;
struct htt_rx_desc *rxd;
@@ -1083,6 +1106,7 @@ static void ath10k_htt_rx_h_undecap_nwifi(struct ath10k *ar,
u8 da[ETH_ALEN];
u8 sa[ETH_ALEN];
int l3_pad_bytes;
+ int bytes_aligned = ar->hw_params.decap_align_bytes;

/* Delivered decapped frame:
* [nwifi 802.11 header] <-- replaced with 802.11 hdr
@@ -1111,6 +1135,14 @@ static void ath10k_htt_rx_h_undecap_nwifi(struct ath10k *ar,
/* push original 802.11 header */
hdr = (struct ieee80211_hdr *)first_hdr;
hdr_len = ieee80211_hdrlen(hdr->frame_control);
+
+ if (!(status->flag & RX_FLAG_IV_STRIPPED)) {
+ memcpy(skb_push(msdu,
+ ath10k_htt_rx_crypto_param_len(ar, enctype)),
+ (void *)hdr + round_up(hdr_len, bytes_aligned),
+ ath10k_htt_rx_crypto_param_len(ar, enctype));
+ }
+
memcpy(skb_push(msdu, hdr_len), hdr, hdr_len);

/* original 802.11 header has a different DA and in
@@ -1171,6 +1203,7 @@ static void ath10k_htt_rx_h_undecap_eth(struct ath10k *ar,
u8 sa[ETH_ALEN];
int l3_pad_bytes;
struct htt_rx_desc *rxd;
+ int bytes_aligned = ar->hw_params.decap_align_bytes;

/* Delivered decapped frame:
* [eth header] <-- replaced with 802.11 hdr & rfc1042/llc
@@ -1199,6 +1232,14 @@ static void ath10k_htt_rx_h_undecap_eth(struct ath10k *ar,
/* push original 802.11 header */
hdr = (struct ieee80211_hdr *)first_hdr;
hdr_len = ieee80211_hdrlen(hdr->frame_control);
+
+ if (!(status->flag & RX_FLAG_IV_STRIPPED)) {
+ memcpy(skb_push(msdu,
+ ath10k_htt_rx_crypto_param_len(ar, enctype)),
+ (void *)hdr + round_up(hdr_len, bytes_aligned),
+ ath10k_htt_rx_crypto_param_len(ar, enctype));
+ }
+
memcpy(skb_push(msdu, hdr_len), hdr, hdr_len);

/* original 802.11 header has a different DA and in
@@ -1212,12 +1253,14 @@ static void ath10k_htt_rx_h_undecap_eth(struct ath10k *ar,
static void ath10k_htt_rx_h_undecap_snap(struct ath10k *ar,
struct sk_buff *msdu,
struct ieee80211_rx_status *status,
- const u8 first_hdr[64])
+ const u8 first_hdr[64],
+ enum htt_rx_mpdu_encrypt_type enctype)
{
struct ieee80211_hdr *hdr;
size_t hdr_len;
int l3_pad_bytes;
struct htt_rx_desc *rxd;
+ int bytes_aligned = ar->hw_params.decap_align_bytes;

/* Delivered decapped frame:
* [amsdu header] <-- replaced with 802.11 hdr
@@ -1233,6 +1276,14 @@ static void ath10k_htt_rx_h_undecap_snap(struct ath10k *ar,

hdr = (struct ieee80211_hdr *)first_hdr;
hdr_len = ieee80211_hdrlen(hdr->frame_control);
+
+ if (!(status->flag & RX_FLAG_IV_STRIPPED)) {
+ memcpy(skb_push(msdu,
+ ath10k_htt_rx_crypto_param_len(ar, enctype)),
+ (void *)hdr + round_up(hdr_len, bytes_aligned),
+ ath10k_htt_rx_crypto_param_len(ar, enctype));
+ }
+
memcpy(skb_push(msdu, hdr_len), hdr, hdr_len);
}

@@ -1267,13 +1318,15 @@ static void ath10k_htt_rx_h_undecap(struct ath10k *ar,
is_decrypted);
break;
case RX_MSDU_DECAP_NATIVE_WIFI:
- ath10k_htt_rx_h_undecap_nwifi(ar, msdu, status, first_hdr);
+ ath10k_htt_rx_h_undecap_nwifi(ar, msdu, status, first_hdr,
+ enctype);
break;
case RX_MSDU_DECAP_ETHERNET2_DIX:
ath10k_htt_rx_h_undecap_eth(ar, msdu, status, first_hdr, enctype);
break;
case RX_MSDU_DECAP_8023_SNAP_LLC:
- ath10k_htt_rx_h_undecap_snap(ar, msdu, status, first_hdr);
+ ath10k_htt_rx_h_undecap_snap(ar, msdu, status, first_hdr,
+ enctype);
break;
}
}
@@ -1316,7 +1369,8 @@ static void ath10k_htt_rx_h_csum_offload(struct sk_buff *msdu)

static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
struct sk_buff_head *amsdu,
- struct ieee80211_rx_status *status)
+ struct ieee80211_rx_status *status,
+ bool fill_crypt_header)
{
struct sk_buff *first;
struct sk_buff *last;
@@ -1326,7 +1380,6 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
enum htt_rx_mpdu_encrypt_type enctype;
u8 first_hdr[64];
u8 *qos;
- size_t hdr_len;
bool has_fcs_err;
bool has_crypto_err;
bool has_tkip_err;
@@ -1351,15 +1404,17 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
* decapped header. It'll be used for undecapping of each MSDU.
*/
hdr = (void *)rxd->rx_hdr_status;
- hdr_len = ieee80211_hdrlen(hdr->frame_control);
- memcpy(first_hdr, hdr, hdr_len);
+ memcpy(first_hdr, hdr, RX_HTT_HDR_STATUS_LEN);

/* Each A-MSDU subframe will use the original header as the base and be
* reported as a separate MSDU so strip the A-MSDU bit from QoS Ctl.
*/
hdr = (void *)first_hdr;
- qos = ieee80211_get_qos_ctl(hdr);
- qos[0] &= ~IEEE80211_QOS_CTL_A_MSDU_PRESENT;
+
+ if (ieee80211_is_data_qos(hdr->frame_control)) {
+ qos = ieee80211_get_qos_ctl(hdr);
+ qos[0] &= ~IEEE80211_QOS_CTL_A_MSDU_PRESENT;
+ }

/* Some attention flags are valid only in the last MSDU. */
last = skb_peek_tail(amsdu);
@@ -1406,9 +1461,14 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
status->flag |= RX_FLAG_DECRYPTED;

if (likely(!is_mgmt))
- status->flag |= RX_FLAG_IV_STRIPPED |
- RX_FLAG_MMIC_STRIPPED;
-}
+ status->flag |= RX_FLAG_MMIC_STRIPPED;
+
+ if (fill_crypt_header)
+ status->flag |= RX_FLAG_MIC_STRIPPED |
+ RX_FLAG_ICV_STRIPPED;
+ else
+ status->flag |= RX_FLAG_IV_STRIPPED;
+ }

skb_queue_walk(amsdu, msdu) {
ath10k_htt_rx_h_csum_offload(msdu);
@@ -1424,6 +1484,9 @@ static void ath10k_htt_rx_h_mpdu(struct ath10k *ar,
if (is_mgmt)
continue;

+ if (fill_crypt_header)
+ continue;
+
hdr = (void *)msdu->data;
hdr->frame_control &= ~__cpu_to_le16(IEEE80211_FCTL_PROTECTED);
}
@@ -1434,6 +1497,9 @@ static void ath10k_htt_rx_h_deliver(struct ath10k *ar,
struct ieee80211_rx_status *status)
{
struct sk_buff *msdu;
+ struct sk_buff *first_subframe;
+
+ first_subframe = skb_peek(amsdu);

while ((msdu = __skb_dequeue(amsdu))) {
/* Setup per-MSDU flags */
@@ -1442,6 +1508,13 @@ static void ath10k_htt_rx_h_deliver(struct ath10k *ar,
else
status->flag |= RX_FLAG_AMSDU_MORE;

+ if (msdu == first_subframe) {
+ first_subframe = NULL;
+ status->flag &= ~RX_FLAG_ALLOW_SAME_PN;
+ } else {
+ status->flag |= RX_FLAG_ALLOW_SAME_PN;
+ }
+
ath10k_process_rx(ar, status, msdu);
}
}
@@ -1584,7 +1657,7 @@ static int ath10k_htt_rx_handle_amsdu(struct ath10k_htt *htt)
ath10k_htt_rx_h_unchain(ar, &amsdu);

ath10k_htt_rx_h_filter(ar, &amsdu, rx_status);
- ath10k_htt_rx_h_mpdu(ar, &amsdu, rx_status);
+ ath10k_htt_rx_h_mpdu(ar, &amsdu, rx_status, true);
ath10k_htt_rx_h_deliver(ar, &amsdu, rx_status);

return num_msdus;
@@ -1923,7 +1996,7 @@ static int ath10k_htt_rx_in_ord_ind(struct ath10k *ar, struct sk_buff *skb,
budget_left -= skb_queue_len(&amsdu);
ath10k_htt_rx_h_ppdu(ar, &amsdu, status, vdev_id);
ath10k_htt_rx_h_filter(ar, &amsdu, status);
- ath10k_htt_rx_h_mpdu(ar, &amsdu, status);
+ ath10k_htt_rx_h_mpdu(ar, &amsdu, status, false);
ath10k_htt_rx_h_deliver(ar, &amsdu, status);
break;
case -EAGAIN:
diff --git a/drivers/net/wireless/ath/ath10k/rx_desc.h b/drivers/net/wireless/ath/ath10k/rx_desc.h
index c1022a1cf855..28da14398951 100644
--- a/drivers/net/wireless/ath/ath10k/rx_desc.h
+++ b/drivers/net/wireless/ath/ath10k/rx_desc.h
@@ -239,6 +239,9 @@ enum htt_rx_mpdu_encrypt_type {
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_CCM256_WPA2 = 8,
+ HTT_RX_MPDU_ENCRYPT_AES_GCMP_WPA2 = 9,
+ HTT_RX_MPDU_ENCRYPT_AES_GCMP256_WPA2 = 10,
};

#define RX_MPDU_START_INFO0_PEER_IDX_MASK 0x000007ff
--
2.7.4


2017-10-31 15:00:30

by Kalle Valo

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

U2ViYXN0aWFuIEdvdHRzY2hhbGwgPHMuZ290dHNjaGFsbEBkZC13cnQuY29tPiB3cml0ZXM6DQoN
Cj4gdGhlIGZvbGxvd2luZyBwYXRjaGxpbmVzIGluIHRoZSB2MyBwYXRjaCBsb29rIHdyb25nDQo+
DQo+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIC8qIElDViAqLw0KPiArwqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoCBpZiAoc3RhdHVzLT5mbGFnICYgUlhfRkxBR19JQ1ZfU1RSSVBQ
RUQgJiYNCj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBlbmN0eXBlICE9
IEhUVF9SWF9NUERVX0VOQ1JZUFRfQUVTX0NDTV9XUEEyKQ0KPiArwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgc2tiX3RyaW0obXNkdSwgbXNkdS0+bGVuIC0NCj4g
K8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgIGF0aDEwa19odHRfcnhfY3J5cHRvX3RhaWxfbGVuKGFyLA0KPiBlbmN0eXBlKSk7DQo+
DQo+DQo+IHRoZSBlbmN0eXBlICE9IHdwYTIgaXNudCBlbm91Z2ggc2luY2UgaXQgYWxzbyBiZWxv
bmdzIHRvIGNjbXAtMjU2LA0KPiBnY21wIG1vZGVzIGV0Yy4NCg0KNC4xNCBkb2Vzbid0IHN1cHBv
cnQgQ0NNUC0yNTYgbm9yIEdDTVAgbW9kZXMgeWV0LiBEaWQgeW91IGNoZWNrIHRoZQ0Kc2VwYXJh
dGUgcGF0Y2ggZm9yIDQuMTU6DQoNCmh0dHBzOi8vcGF0Y2h3b3JrLmtlcm5lbC5vcmcvcGF0Y2gv
MTAwMjkwOTcvDQoNCi0tIA0KS2FsbGUgVmFsbw0K

2017-10-27 15:44:53

by Kalle Valo

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

Kalle Valo <[email protected]> wrote:

> 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 rx_hdr_status tlv available in the
> rx descriptor of the first msdu. 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. Hw QCA988X,
> QCA9887, QCA99X0, QCA9984, QCA9888 and QCA4019 currently need the
> rebuilding of cipher header to perform PN/TSC check for replay
> attack.
>
> Please note that removing crypto tail for CCMP-256, GCMP and GCMP-256 ciphers
> in raw mode needs to be fixed. Since Rx with these ciphers in raw
> mode does not work in the current form even without this patch and
> removing crypto tail for these chipers needs clean up, raw mode related
> issues in CCMP-256, GCMP and GCMP-256 can be addressed in follow up
> patches.
>
> Tested-by: Manikanta Pubbisetty <[email protected]>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>

Any comments or feedback about this? I'm hoping to apply this during the
weekend so that it will make it to v4.14 in time. So if there are any issues
with this patch please let me know ASAP.

--
https://patchwork.kernel.org/patch/10028621/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

=0A=
Sorry top posting.=0A=
=0A=
The issues in raw mode with CCMP-256, GCMP and GCMP-256 were already known =
and=0A=
the same was captured in the commit log. As mentioned in the commit log, ra=
w mode=0A=
with these ciphers does not work even without this particular patch and it =
needs some cleanup=0A=
like done in the follow up patch https://patchwork.kernel.org/patch/1002909=
9/.=0A=
=0A=
Vasanth=0A=
________________________________________=0A=
From: Sebastian Gottschall <[email protected]>=0A=
Sent: Tuesday, October 31, 2017 8:24 PM=0A=
To: Kalle Valo=0A=
Cc: [email protected]; [email protected]; Vasanthakum=
ar Thiagarajan=0A=
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames=0A=
=0A=
the same is for the MIC=0A=
=0A=
+ /* MIC */=0A=
+ if ((status->flag & RX_FLAG_MIC_STRIPPED) &&=0A=
+ enctype =3D=3D HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2)=0A=
+ skb_trim(msdu, msdu->len - 8);=0A=
=0A=
=0A=
this code looks wrong too=0A=
=0A=
Am 30.10.2017 um 10:32 schrieb Sebastian Gottschall:=0A=
> will check it tomorrow including gcmp-256, ccmp-256. was out for=0A=
> weekend :-)=0A=
>=0A=
> Am 30.10.2017 um 09:39 schrieb Kalle Valo:=0A=
>> Kalle Valo <[email protected]> wrote:=0A=
>>=0A=
>>> Rx data frames notified through HTT_T2H_MSG_TYPE_RX_IND and=0A=
>>> HTT_T2H_MSG_TYPE_RX_FRAG_IND expect PN/TSC check to be done=0A=
>>> on host (mac80211) rather than firmware. Rebuild cipher header=0A=
>>> in every received data frames (that are notified through those=0A=
>>> HTT interfaces) from the rx_hdr_status tlv available in the=0A=
>>> rx descriptor of the first msdu. Skip setting RX_FLAG_IV_STRIPPED=0A=
>>> flag for the packets which requires mac80211 PN/TSC check support=0A=
>>> and set appropriate RX_FLAG for stripped crypto tail. Hw QCA988X,=0A=
>>> QCA9887, QCA99X0, QCA9984, QCA9888 and QCA4019 currently need the=0A=
>>> rebuilding of cipher header to perform PN/TSC check for replay=0A=
>>> attack.=0A=
>>>=0A=
>>> Please note that removing crypto tail for CCMP-256, GCMP and=0A=
>>> GCMP-256 ciphers=0A=
>>> in raw mode needs to be fixed. Since Rx with these ciphers in raw=0A=
>>> mode does not work in the current form even without this patch and=0A=
>>> removing crypto tail for these chipers needs clean up, raw mode related=
=0A=
>>> issues in CCMP-256, GCMP and GCMP-256 can be addressed in follow up=0A=
>>> patches.=0A=
>>>=0A=
>>> Tested-by: Manikanta Pubbisetty <[email protected]>=0A=
>>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>=0A=
>>> Signed-off-by: Kalle Valo <[email protected]>=0A=
>> Patch applied to ath-current branch of ath.git, thanks.=0A=
>>=0A=
>> 7eccb738fce5 ath10k: rebuild crypto header in rx data frames=0A=
>>=0A=
>=0A=
=0A=
--=0A=
Mit freundlichen Gr=FCssen / Regards=0A=
=0A=
Sebastian Gottschall / CTO=0A=
=0A=
NewMedia-NET GmbH - DD-WRT=0A=
Firmensitz: Stubenwaldallee 21a, 64625 Bensheim=0A=
Registergericht: Amtsgericht Darmstadt, HRB 25473=0A=
Gesch=E4ftsf=FChrer: Peter Steinh=E4user, Christian Scheele=0A=
http://www.dd-wrt.com=0A=
email: [email protected]=0A=
Tel.: +496251-582650 / Fax: +496251-5826565=0A=
=0A=

2017-10-30 08:39:23

by Kalle Valo

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

Kalle Valo <[email protected]> wrote:

> 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 rx_hdr_status tlv available in the
> rx descriptor of the first msdu. 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. Hw QCA988X,
> QCA9887, QCA99X0, QCA9984, QCA9888 and QCA4019 currently need the
> rebuilding of cipher header to perform PN/TSC check for replay
> attack.
>
> Please note that removing crypto tail for CCMP-256, GCMP and GCMP-256 ciphers
> in raw mode needs to be fixed. Since Rx with these ciphers in raw
> mode does not work in the current form even without this patch and
> removing crypto tail for these chipers needs clean up, raw mode related
> issues in CCMP-256, GCMP and GCMP-256 can be addressed in follow up
> patches.
>
> Tested-by: Manikanta Pubbisetty <[email protected]>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>

Patch applied to ath-current branch of ath.git, thanks.

7eccb738fce5 ath10k: rebuild crypto header in rx data frames

--
https://patchwork.kernel.org/patch/10028621/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2017-10-31 14:52:19

by Sebastian Gottschall

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

the following patchlines in the v3 patch look wrong

+               /* 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));


the enctype != wpa2 isnt enough since it also belongs to ccmp-256, gcmp
modes etc.

my proposal

       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_TKIP_WPA:
               skb_trim(msdu, msdu->len -
                        ath10k_htt_rx_crypto_tail_len(ar, enctype));
           break;
           default:
           break;
          }
-     }


Am 30.10.2017 um 10:32 schrieb Sebastian Gottschall:
> will check it tomorrow including gcmp-256, ccmp-256. was out for
> weekend :-)
>
> Am 30.10.2017 um 09:39 schrieb Kalle Valo:
>> Kalle Valo <[email protected]> wrote:
>>
>>> 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 rx_hdr_status tlv available in the
>>> rx descriptor of the first msdu. 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. Hw QCA988X,
>>> QCA9887, QCA99X0, QCA9984, QCA9888 and QCA4019 currently need the
>>> rebuilding of cipher header to perform PN/TSC check for replay
>>> attack.
>>>
>>> Please note that removing crypto tail for CCMP-256, GCMP and
>>> GCMP-256 ciphers
>>> in raw mode needs to be fixed. Since Rx with these ciphers in raw
>>> mode does not work in the current form even without this patch and
>>> removing crypto tail for these chipers needs clean up, raw mode related
>>> issues in CCMP-256, GCMP and GCMP-256 can be addressed in follow up
>>> patches.
>>>
>>> Tested-by: Manikanta Pubbisetty <[email protected]>
>>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>>> Signed-off-by: Kalle Valo <[email protected]>
>> Patch applied to ath-current branch of ath.git, thanks.
>>
>> 7eccb738fce5 ath10k: rebuild crypto header in rx data frames
>>
>

--
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: [email protected]
Tel.: +496251-582650 / Fax: +496251-5826565

2017-10-31 15:07:40

by Sebastian Gottschall

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

Am 31.10.2017 um 16:00 schrieb Kalle Valo:
> Sebastian Gottschall <[email protected]> writes:
>
>> the following patchlines in the v3 patch look wrong
>>
>> +               /* 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));
>>
>>
>> the enctype != wpa2 isnt enough since it also belongs to ccmp-256,
>> gcmp modes etc.
> 4.14 doesn't support CCMP-256 nor GCMP modes yet. Did you check the
> separate patch for 4.15:

for me ccmp-256 and gcmp works but my current tree is based on a very
recent mac80211 version

but did not check if this support is included in 4.14 since i'm
developing embedded wireless firmwares on different kernel versions
using wireless backports based on recent mac80211 /ath10k etc. versions

>
> https://patchwork.kernel.org/patch/10029097/
thanks for notifying me. havent seen it
>

--
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: [email protected]
Tel.: +496251-582650 / Fax: +496251-5826565

2017-10-30 09:32:21

by Sebastian Gottschall

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

will check it tomorrow including gcmp-256, ccmp-256. was out for weekend :-)

Am 30.10.2017 um 09:39 schrieb Kalle Valo:
> Kalle Valo <[email protected]> wrote:
>
>> 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 rx_hdr_status tlv available in the
>> rx descriptor of the first msdu. 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. Hw QCA988X,
>> QCA9887, QCA99X0, QCA9984, QCA9888 and QCA4019 currently need the
>> rebuilding of cipher header to perform PN/TSC check for replay
>> attack.
>>
>> Please note that removing crypto tail for CCMP-256, GCMP and GCMP-256 ciphers
>> in raw mode needs to be fixed. Since Rx with these ciphers in raw
>> mode does not work in the current form even without this patch and
>> removing crypto tail for these chipers needs clean up, raw mode related
>> issues in CCMP-256, GCMP and GCMP-256 can be addressed in follow up
>> patches.
>>
>> Tested-by: Manikanta Pubbisetty <[email protected]>
>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>> Signed-off-by: Kalle Valo <[email protected]>
> Patch applied to ath-current branch of ath.git, thanks.
>
> 7eccb738fce5 ath10k: rebuild crypto header in rx data frames
>

--
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: [email protected]
Tel.: +496251-582650 / Fax: +496251-5826565

2017-10-31 14:54:55

by Sebastian Gottschall

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

the same is for the MIC

+               /* MIC */
+               if ((status->flag & RX_FLAG_MIC_STRIPPED) &&
+                   enctype == HTT_RX_MPDU_ENCRYPT_AES_CCM_WPA2)
+                       skb_trim(msdu, msdu->len - 8);


this code looks wrong too

Am 30.10.2017 um 10:32 schrieb Sebastian Gottschall:
> will check it tomorrow including gcmp-256, ccmp-256. was out for
> weekend :-)
>
> Am 30.10.2017 um 09:39 schrieb Kalle Valo:
>> Kalle Valo <[email protected]> wrote:
>>
>>> 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 rx_hdr_status tlv available in the
>>> rx descriptor of the first msdu. 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. Hw QCA988X,
>>> QCA9887, QCA99X0, QCA9984, QCA9888 and QCA4019 currently need the
>>> rebuilding of cipher header to perform PN/TSC check for replay
>>> attack.
>>>
>>> Please note that removing crypto tail for CCMP-256, GCMP and
>>> GCMP-256 ciphers
>>> in raw mode needs to be fixed. Since Rx with these ciphers in raw
>>> mode does not work in the current form even without this patch and
>>> removing crypto tail for these chipers needs clean up, raw mode related
>>> issues in CCMP-256, GCMP and GCMP-256 can be addressed in follow up
>>> patches.
>>>
>>> Tested-by: Manikanta Pubbisetty <[email protected]>
>>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>>> Signed-off-by: Kalle Valo <[email protected]>
>> Patch applied to ath-current branch of ath.git, thanks.
>>
>> 7eccb738fce5 ath10k: rebuild crypto header in rx data frames
>>
>

--
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: [email protected]
Tel.: +496251-582650 / Fax: +496251-5826565

2017-11-21 18:54:06

by Ben Greear

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

Is this patch destined for stable?

I was expecting to see it show up in the older stable releases,
but it has not so far as far as I can tell.

Thanks,
Ben

On 10/30/2017 02:32 AM, Sebastian Gottschall wrote:
> will check it tomorrow including gcmp-256, ccmp-256. was out for weekend :-)
>
> Am 30.10.2017 um 09:39 schrieb Kalle Valo:
>> Kalle Valo <[email protected]> wrote:
>>
>>> 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 rx_hdr_status tlv available in the
>>> rx descriptor of the first msdu. 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. Hw QCA988X,
>>> QCA9887, QCA99X0, QCA9984, QCA9888 and QCA4019 currently need the
>>> rebuilding of cipher header to perform PN/TSC check for replay
>>> attack.
>>>
>>> Please note that removing crypto tail for CCMP-256, GCMP and GCMP-256 ciphers
>>> in raw mode needs to be fixed. Since Rx with these ciphers in raw
>>> mode does not work in the current form even without this patch and
>>> removing crypto tail for these chipers needs clean up, raw mode related
>>> issues in CCMP-256, GCMP and GCMP-256 can be addressed in follow up
>>> patches.
>>>
>>> Tested-by: Manikanta Pubbisetty <[email protected]>
>>> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
>>> Signed-off-by: Kalle Valo <[email protected]>
>> Patch applied to ath-current branch of ath.git, thanks.
>>
>> 7eccb738fce5 ath10k: rebuild crypto header in rx data frames
>>
>


--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com

2017-12-03 07:36:59

by Sebastian Gottschall

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

you need a recent ath10k version. use the backports project to compile a
recent ath10k/mac80211 for 3.17
or take a look into lede, it has a complete recent backports tree which
can be used on 3.17 with small modifications

Am 02.12.2017 um 22:41 schrieb Ben Greear:
>
>
> On 12/02/2017 06:34 AM, Kalle Valo wrote:
>> Ben Greear <[email protected]> writes:
>>
>>> Is this patch destined for stable?
>>>
>>> I was expecting to see it show up in the older stable releases,
>>> but it has not so far as far as I can tell.
>>
>> We are working on it, but not sure yet to which releases, it doesn't
>> directly apply to all active stable releases.
>
> I had no luck trying to apply it to a 3.17-ish ath10k.? Gave up after
> a bit and tried backporting the whole stack instead :P
>
> Thanks,
> Ben
>

--
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: [email protected]
Tel.: +496251-582650 / Fax: +496251-5826565

2017-12-02 14:34:07

by Kalle Valo

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames

Ben Greear <[email protected]> writes:

> Is this patch destined for stable?
>
> I was expecting to see it show up in the older stable releases,
> but it has not so far as far as I can tell.

We are working on it, but not sure yet to which releases, it doesn't
directly apply to all active stable releases.

--=20
Kalle Valo=

2017-12-02 21:41:17

by Ben Greear

[permalink] [raw]
Subject: Re: [v3] ath10k: rebuild crypto header in rx data frames



On 12/02/2017 06:34 AM, Kalle Valo wrote:
> Ben Greear <[email protected]> writes:
>
>> Is this patch destined for stable?
>>
>> I was expecting to see it show up in the older stable releases,
>> but it has not so far as far as I can tell.
>
> We are working on it, but not sure yet to which releases, it doesn't
> directly apply to all active stable releases.

I had no luck trying to apply it to a 3.17-ish ath10k. Gave up after
a bit and tried backporting the whole stack instead :P

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com