[PATCH] mac80211: consider Order bit to fill CCMP AAD

[PATCH] mac80211: consider Order bit to fill CCMP AAD

[Ping-Ke Shih]

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(-)

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
-- 
2.25.1

Re: [PATCH] mac80211: consider Order bit to fill CCMP AAD

[Johannes Berg]

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

RE: [PATCH] mac80211: consider Order bit to fill CCMP AAD

[Pkshih]

> -----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

Re: [PATCH] mac80211: consider Order bit to fill CCMP AAD

[Johannes Berg]

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

RE: [PATCH] mac80211: consider Order bit to fill CCMP AAD

[Pkshih]

> -----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

Re: [PATCH] mac80211: consider Order bit to fill CCMP AAD

[Jouni Malinen]

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.

-- 
Jouni Malinen                                            PGP id EFC895FA