| Message ID | 20220324004816.6202-1-pkshih@realtek.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: consider Order bit to fill CCMP AAD | expand |
On Thu, 2022-03-24 at 08:48 +0800, Ping-Ke Shih wrote: > Follow IEEE 802.11-21 that HTC subfield masked to 0 for all data frames > containing a QoS Control field. > That seems fine, though not sure it's actually _relevant_ - how would we possibly generate such frames in mac80211? > It also defines the AAD length depends on > QC and A4 fields, so change logic to determine length accordingly. This I don't understand. The code > - hdrlen = ieee80211_hdrlen(hdr->frame_control); > - len_a = hdrlen - 2; sets it to the same thing, no? Oh, I see - again you're worried about IEEE80211_HT_CTL_LEN I guess? Maybe just subtract that again? But either way, I'm not sure how we'd generate these in the first place. johannes
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Monday, April 11, 2022 5:12 PM > To: Pkshih <pkshih@realtek.com> > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH] mac80211: consider Order bit to fill CCMP AAD > > On Thu, 2022-03-24 at 08:48 +0800, Ping-Ke Shih wrote: > > Follow IEEE 802.11-21 that HTC subfield masked to 0 for all data frames > > containing a QoS Control field. > > > > That seems fine, though not sure it's actually _relevant_ - how would we > possibly generate such frames in mac80211? I suddenly meet this case, because hardware decryption isn't configured properly during development, so it falls into software decryption. The received packets can possibly contain HTC. After I fix my driver, I don't need this, but I think it is worth to have this patch. > > > It also defines the AAD length depends on > > QC and A4 fields, so change logic to determine length accordingly. > > This I don't understand. IEEE 802.11-21 illustrate the use cases as below QC Field A4 Field AAD length ------- -------- --------- x x 22 o x 24 x o 28 o o 30 I write logic along with this table, because it would be clear and simple. > > The code > > > > - hdrlen = ieee80211_hdrlen(hdr->frame_control); > > - len_a = hdrlen - 2; > > sets it to the same thing, no? Since AAD consists of "FC | A1 | A2 | A3 | SC | [A4] | [QC]" I think '-2' here means AAD doesn't contain 'Duration' field. > > Oh, I see - again you're worried about IEEE80211_HT_CTL_LEN I guess? > > Maybe just subtract that again? Yes, we can subtract length from ieee80211_hdrlen(). But, this function is called in data path that means every packet can use it. Is it reasonable to += IEEE80211_HT_CTL_LEN in ieee80211_hdrlen() and -= IEEE80211_HT_CTL_LEN right after leaving ieee80211_hdrlen() if the packet is ieee80211_has_order()? Ping-Ke
On Tue, 2022-04-12 at 00:37 +0000, Pkshih wrote: > > > > That seems fine, though not sure it's actually _relevant_ - how > > would we > > possibly generate such frames in mac80211? > > I suddenly meet this case, because hardware decryption isn't > configured properly > during development, so it falls into software decryption. The received > packets > can possibly contain HTC. Haha, right. I'm an idiot I guess, why did I not think of RX? > After I fix my driver, I don't need this, but I think it is worth to > have > this patch. OK. But I guess that means no hurry. > > > > Oh, I see - again you're worried about IEEE80211_HT_CTL_LEN I guess? > > > > Maybe just subtract that again? > > Yes, we can subtract length from ieee80211_hdrlen(). > But, this function is called in data path that means every packet can > use it. > Is it reasonable to += IEEE80211_HT_CTL_LEN in ieee80211_hdrlen() and > -= IEEE80211_HT_CTL_LEN right after leaving ieee80211_hdrlen() if the > packet is > ieee80211_has_order()? Yeah, thinking about that, I guess you're right. Maybe we can express the 22 a bit better (some headerlen - 2 with a comment?), but I can look at that when I apply the patch. Thanks! johannes
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Tuesday, April 12, 2022 1:47 PM > To: Pkshih <pkshih@realtek.com> > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH] mac80211: consider Order bit to fill CCMP AAD > > On Tue, 2022-04-12 at 00:37 +0000, Pkshih wrote: > > After I fix my driver, I don't need this, but I think it is worth to > > have > > this patch. > > OK. But I guess that means no hurry. Yes. > > Yeah, thinking about that, I guess you're right. Maybe we can express > the 22 a bit better (some headerlen - 2 with a comment?), but I can look > at that when I apply the patch. How about replacing 22 by 2 + 6 + 6 + 6 + 2 because of "FC | A1 | A2 | A3 | SC | [A4] | [QC]" ? Ping-Ke
On Thu, Mar 24, 2022 at 08:48:16AM +0800, Ping-Ke Shih wrote: > Follow IEEE 802.11-21 that HTC subfield masked to 0 for all data frames > containing a QoS Control field. It also defines the AAD length depends on > QC and A4 fields, so change logic to determine length accordingly. > diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c > @@ -317,13 +317,12 @@ static void ccmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *b_0, u8 *aad) > /* > * Mask FC: zero subtype b4 b5 b6 (if not mgmt) > - * Retry, PwrMgt, MoreData; set Protected > + * Retry, PwrMgt, MoreData, Order (if Qos Data); set Protected > */ ... For completeness, we should really do the same got GCMP AAD which is identical to the CCMP AAD. In other words, these changes should be done in gcmp_special_blocks() as well. Those functions should really have next to identical implementation for the AAD part (nonce construction is different, though). There were already some differences in the design before.. Maybe all this AAD stuff should really be moved into a separate helper function that both CCMP and GCMP could use.
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c index 7ed0d268aff2f..cd35ae76d5b78 100644 --- a/net/mac80211/wpa.c +++ b/net/mac80211/wpa.c @@ -317,13 +317,12 @@ static void ccmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *b_0, u8 *aad) __le16 mask_fc; int a4_included, mgmt; u8 qos_tid; - u16 len_a; - unsigned int hdrlen; + u16 len_a = 22; struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data; /* * Mask FC: zero subtype b4 b5 b6 (if not mgmt) - * Retry, PwrMgt, MoreData; set Protected + * Retry, PwrMgt, MoreData, Order (if Qos Data); set Protected */ mgmt = ieee80211_is_mgmt(hdr->frame_control); mask_fc = hdr->frame_control; @@ -333,14 +332,17 @@ static void ccmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *b_0, u8 *aad) mask_fc &= ~cpu_to_le16(0x0070); mask_fc |= cpu_to_le16(IEEE80211_FCTL_PROTECTED); - hdrlen = ieee80211_hdrlen(hdr->frame_control); - len_a = hdrlen - 2; a4_included = ieee80211_has_a4(hdr->frame_control); + if (a4_included) + len_a += 6; - if (ieee80211_is_data_qos(hdr->frame_control)) + if (ieee80211_is_data_qos(hdr->frame_control)) { qos_tid = ieee80211_get_tid(hdr); - else + mask_fc &= ~cpu_to_le16(IEEE80211_FCTL_ORDER); + len_a += 2; + } else { qos_tid = 0; + } /* In CCM, the initial vectors (IV) used for CTR mode encryption and CBC * mode authentication are not allowed to collide, yet both are derived
Follow IEEE 802.11-21 that HTC subfield masked to 0 for all data frames containing a QoS Control field. It also defines the AAD length depends on QC and A4 fields, so change logic to determine length accordingly. Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> --- net/mac80211/wpa.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)