Re: [PATCH 8/8] mt76: validate rx CCMP PN

[Felix Fietkau]

On 2018-01-24 17:20, Johannes Berg wrote:
> On Wed, 2018-01-24 at 17:11 +0100, Felix Fietkau wrote:
>> 
>> I guess I will have to look into fragmentation. I have a second driver
>> pending that only reports the CCMP PN outside of the packet, and for
>> performance reasons I really don't want to translate it and move it to a
>> place where mac80211 can parse it.
>> I'm also looking into doing parallel rx in software to see if I can get
>> more performance that way. I think for that I would also need CCMP PN
>> validation in the driver.
> 
> Fair enough. We also do it, except we just decide that fragmented
> packets we don't care and can use mac80211 - but we get the PN in-
> frame, just can't always use mac80211 due to parallel RX.
I guess I could make the driver translate the header for fragmented
packets only.

- Felix

Re: [PATCH 8/8] mt76: validate rx CCMP PN

[Johannes Berg]

On Wed, 2018-01-24 at 17:11 +0100, Felix Fietkau wrote:
> 
> I guess I will have to look into fragmentation. I have a second driver
> pending that only reports the CCMP PN outside of the packet, and for
> performance reasons I really don't want to translate it and move it to a
> place where mac80211 can parse it.
> I'm also looking into doing parallel rx in software to see if I can get
> more performance that way. I think for that I would also need CCMP PN
> validation in the driver.

Fair enough. We also do it, except we just decide that fragmented
packets we don't care and can use mac80211 - but we get the PN in-
frame, just can't always use mac80211 due to parallel RX.

johannes

Re: [PATCH 8/8] mt76: validate rx CCMP PN

[Johannes Berg]

On Wed, 2018-01-24 at 16:19 +0100, Felix Fietkau wrote:
> Apparently hardware does not perform CCMP PN validation in hardware, so
> we need to take care of this in the driver. This is important for
> protecting against replay attacks
> 
> +static int
> +mt76_check_ccmp_pn(struct sk_buff *skb)
> +{
> + struct mt76_rx_status *status = (struct mt76_rx_status *) skb->cb;
> + struct mt76_wcid *wcid = status->wcid;
> + int ret;
> +
> + if (!(status->flag & RX_FLAG_DECRYPTED))
> + return 0;
> +
> + if (!wcid || !wcid->rx_check_pn)
> + return 0;
> +
> + BUILD_BUG_ON(sizeof(status->iv) != sizeof(wcid->rx_key_pn[0]));
> + ret = memcmp(status->iv, wcid->rx_key_pn[status->tid],
> +  sizeof(status->iv));
> + if (ret <= 0)
> + return -EINVAL; /* replay */
> +
> + memcpy(wcid->rx_key_pn[status->tid], status->iv, sizeof(status->iv));
> + status->flag |= RX_FLAG_PN_VALIDATED;

You shouldn't do this, try to somehow make it rely on mac80211 instead.

Otherwise, you really have to handle CCMP vs. fragmentation.

johannes

[PATCH 8/8] mt76: validate rx CCMP PN

[Felix Fietkau]

Apparently hardware does not perform CCMP PN validation in hardware, so
we need to take care of this in the driver. This is important for
protecting against replay attacks

Signed-off-by: Felix Fietkau 
---
 drivers/net/wireless/mediatek/mt76/mac80211.c| 51 
 drivers/net/wireless/mediatek/mt76/mt76.h| 12 +-
 drivers/net/wireless/mediatek/mt76/mt76x2_init.c |  2 +-
 drivers/net/wireless/mediatek/mt76/mt76x2_mac.c  | 45 -
 drivers/net/wireless/mediatek/mt76/mt76x2_main.c |  1 +
 5 files changed, 98 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c 
b/drivers/net/wireless/mediatek/mt76/mac80211.c
index 77f1be161009..c203e7b48c58 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -384,6 +384,27 @@ int mt76_get_survey(struct ieee80211_hw *hw, int idx,
 }
 EXPORT_SYMBOL_GPL(mt76_get_survey);
 
+void mt76_wcid_key_setup(struct mt76_dev *dev, struct mt76_wcid *wcid,
+struct ieee80211_key_conf *key)
+{
+   struct ieee80211_key_seq seq;
+   int i;
+
+   wcid->rx_check_pn = false;
+
+   if (!key)
+   return;
+
+   if (key->cipher == WLAN_CIPHER_SUITE_CCMP)
+   wcid->rx_check_pn = true;
+
+   for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
+   ieee80211_get_key_rx_seq(key, i, );
+   memcpy(wcid->rx_key_pn[i], seq.ccmp.pn, sizeof(seq.ccmp.pn));
+   }
+}
+EXPORT_SYMBOL(mt76_wcid_key_setup);
+
 static struct ieee80211_sta *mt76_rx_convert(struct sk_buff *skb)
 {
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
@@ -410,6 +431,31 @@ static struct ieee80211_sta *mt76_rx_convert(struct 
sk_buff *skb)
return wcid_to_sta(mstat.wcid);
 }
 
+static int
+mt76_check_ccmp_pn(struct sk_buff *skb)
+{
+   struct mt76_rx_status *status = (struct mt76_rx_status *) skb->cb;
+   struct mt76_wcid *wcid = status->wcid;
+   int ret;
+
+   if (!(status->flag & RX_FLAG_DECRYPTED))
+   return 0;
+
+   if (!wcid || !wcid->rx_check_pn)
+   return 0;
+
+   BUILD_BUG_ON(sizeof(status->iv) != sizeof(wcid->rx_key_pn[0]));
+   ret = memcmp(status->iv, wcid->rx_key_pn[status->tid],
+sizeof(status->iv));
+   if (ret <= 0)
+   return -EINVAL; /* replay */
+
+   memcpy(wcid->rx_key_pn[status->tid], status->iv, sizeof(status->iv));
+   status->flag |= RX_FLAG_PN_VALIDATED;
+
+   return 0;
+}
+
 void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
  int queue)
 {
@@ -421,6 +467,11 @@ void mt76_rx_complete(struct mt76_dev *dev, struct 
sk_buff_head *frames,
napi = >napi[queue];
 
while ((skb = __skb_dequeue(frames)) != NULL) {
+   if (mt76_check_ccmp_pn(skb)) {
+   dev_kfree_skb(skb);
+   continue;
+   }
+
sta = mt76_rx_convert(skb);
ieee80211_rx_napi(dev->hw, sta, skb, napi);
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h 
b/drivers/net/wireless/mediatek/mt76/mt76.h
index af98bc65c2e1..129015c9d116 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -131,6 +131,9 @@ struct mt76_wcid {
 
u8 sta:1;
 
+   u8 rx_check_pn;
+   u8 rx_key_pn[IEEE80211_NUM_TIDS][6];
+
__le16 tx_rate;
bool tx_rate_set;
u8 tx_rate_nss;
@@ -279,12 +282,14 @@ struct mt76_rx_status {
 
unsigned long reorder_time;
 
-   u8 aggr;
+   u8 iv[6];
+
+   u8 aggr:1;
u8 tid;
u16 seqno;
 
-   u32 flag;
u16 freq;
+   u32 flag;
u8 enc_flags;
u8 encoding:2, bw:3;
u8 rate_idx;
@@ -413,6 +418,9 @@ int mt76_rx_aggr_start(struct mt76_dev *dev, struct 
mt76_wcid *wcid, u8 tid,
   u16 ssn, u8 size);
 void mt76_rx_aggr_stop(struct mt76_dev *dev, struct mt76_wcid *wcid, u8 tid);
 
+void mt76_wcid_key_setup(struct mt76_dev *dev, struct mt76_wcid *wcid,
+struct ieee80211_key_conf *key);
+
 /* internal */
 void mt76_tx_free(struct mt76_dev *dev);
 void mt76_put_txwi(struct mt76_dev *dev, struct mt76_txwi_cache *t);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2_init.c 
b/drivers/net/wireless/mediatek/mt76/mt76x2_init.c
index 1e34b578b151..1b00ae4465a2 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2_init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2_init.c
@@ -131,7 +131,7 @@ mt76_write_mac_initvals(struct mt76x2_dev *dev)
{ MT_RX_FILTR_CFG,  0x00015f97 },
{ MT_LEGACY_BASIC_RATE, 0x017f },
{ MT_HT_BASIC_RATE, 0x4003 },
-   { MT_PN_PAD_MODE,   0x0002 },
+   { MT_PN_PAD_MODE,