2022-09-22 20:45:16

by Marek Vasut

[permalink] [raw]
Subject: [PATCH] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

When using wpa_supplicant v2.10, this driver is no longer able to
associate with any AP and fails in the EAPOL 4-way handshake while
sending the 2/4 message to the AP. The problem is not present in
wpa_supplicant v2.9 or older. The problem stems from HostAP commit
144314eaa ("wpa_supplicant: Send EAPOL frames over nl80211 where available")
which changes the way EAPOL frames are sent, from them being send
at L2 frames to them being sent via nl80211 control port.

An EAPOL frame sent as L2 frame is passed to the WiFi driver with
skb->protocol ETH_P_PAE, while EAPOL frame sent via nl80211 control
port has skb->protocol set to ETH_P_802_3 . The later happens in
ieee80211_tx_control_port(), where the EAPOL frame is encapsulated
into 802.3 frame.

The rsi_91x driver handles ETH_P_PAE EAPOL frames as high-priority
frames and sends them via highest-priority transmit queue, while
the ETH_P_802_3 frames are sent as regular frames. The EAPOL 4-way
handshake frames must be sent as highest-priority, otherwise the
4-way handshake times out.

Therefore, to fix this problem, inspect the ETH_P_802_3 frames in
the rsi_91x driver, check the ethertype of the encapsulated frame,
and in case it is ETH_P_PAE, transmit the frame via high-priority
queue just like other ETH_P_PAE frames.

Fixes: 0eb42586cf876 ("rsi: data packet descriptor enhancements")
Signed-off-by: Marek Vasut <[email protected]>
---
NOTE: I am really unsure about the method of finding out the exact
place of ethernet header in the encapsulated packet and then
extracting the ethertype from it. Is there maybe some sort of
helper function for that purpose ?
---
Cc: Amitkumar Karwar <[email protected]>
Cc: Angus Ainslie <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Martin Fuzzey <[email protected]>
Cc: Martin Kepplinger <[email protected]>
Cc: Prameela Rani Garnepudi <[email protected]>
Cc: Sebastian Krzyszkowiak <[email protected]>
Cc: Siva Rebbagondla <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/net/wireless/rsi/rsi_91x_core.c | 14 ++++++++++++++
drivers/net/wireless/rsi/rsi_91x_hal.c | 15 ++++++++++++++-
2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_core.c b/drivers/net/wireless/rsi/rsi_91x_core.c
index 0f3a80f66b61c..d76c9dc99cafa 100644
--- a/drivers/net/wireless/rsi/rsi_91x_core.c
+++ b/drivers/net/wireless/rsi/rsi_91x_core.c
@@ -380,6 +380,9 @@ void rsi_core_xmit(struct rsi_common *common, struct sk_buff *skb)
struct ieee80211_vif *vif;
u8 q_num, tid = 0;
struct rsi_sta *rsta = NULL;
+ struct ethhdr *eth_hdr;
+ bool tx_eapol = false;
+ unsigned int hdr_len;

if ((!skb) || (!skb->len)) {
rsi_dbg(ERR_ZONE, "%s: Null skb/zero Length packet\n",
@@ -466,7 +469,18 @@ void rsi_core_xmit(struct rsi_common *common, struct sk_buff *skb)
tid, 0);
}
}
+
if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
+ tx_eapol = true;
+ } else if (skb->protocol == cpu_to_be16(ETH_P_802_3)) {
+ hdr_len = ieee80211_get_hdrlen_from_skb(skb) +
+ sizeof(rfc1042_header) - ETH_HLEN + 2;
+ eth_hdr = (struct ethhdr *)(skb->data + hdr_len);
+ if (eth_hdr->h_proto == cpu_to_be16(ETH_P_PAE))
+ tx_eapol = true;
+ }
+
+ if (tx_eapol) {
q_num = MGMT_SOFT_Q;
skb->priority = q_num;
}
diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
index c61f83a7333b6..d43754fff287d 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -159,6 +159,9 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
struct rsi_data_desc *data_desc;
struct rsi_xtended_desc *xtend_desc;
u8 ieee80211_size = MIN_802_11_HDR_LEN;
+ struct ethhdr *eth_hdr;
+ bool tx_eapol = false;
+ unsigned int hdr_len;
u8 header_size;
u8 vap_id = 0;
u8 dword_align_bytes;
@@ -168,6 +171,16 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
vif = info->control.vif;
tx_params = (struct skb_info *)info->driver_data;

+ if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
+ tx_eapol = true;
+ } else if (skb->protocol == cpu_to_be16(ETH_P_802_3)) {
+ hdr_len = ieee80211_get_hdrlen_from_skb(skb) +
+ sizeof(rfc1042_header) - ETH_HLEN + 2;
+ eth_hdr = (struct ethhdr *)(skb->data + hdr_len);
+ if (eth_hdr->h_proto == cpu_to_be16(ETH_P_PAE))
+ tx_eapol = true;
+ }
+
header_size = FRAME_DESC_SZ + sizeof(struct rsi_xtended_desc);
if (header_size > skb_headroom(skb)) {
rsi_dbg(ERR_ZONE, "%s: Unable to send pkt\n", __func__);
@@ -231,7 +244,7 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
}
}

- if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
+ if (tx_eapol) {
rsi_dbg(INFO_ZONE, "*** Tx EAPOL ***\n");

data_desc->frame_info = cpu_to_le16(RATE_INFO_ENABLE);
--
2.35.1


2022-09-23 07:17:50

by Martin Fuzzey

[permalink] [raw]
Subject: Re: [PATCH] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

Hi Marek,

On Thu, 22 Sept 2022 at 22:33, Marek Vasut <[email protected]> wrote:
>
> When using wpa_supplicant v2.10, this driver is no longer able to
> associate with any AP and fails in the EAPOL 4-way handshake while
> sending the 2/4 message to the AP. The problem is not present in
> wpa_supplicant v2.9 or older. The problem stems from HostAP commit
> 144314eaa ("wpa_supplicant: Send EAPOL frames over nl80211 where available")
> which changes the way EAPOL frames are sent, from them being send
> at L2 frames to them being sent via nl80211 control port.
...
> Therefore, to fix this problem, inspect the ETH_P_802_3 frames in
> the rsi_91x driver, check the ethertype of the encapsulated frame,
> and in case it is ETH_P_PAE, transmit the frame via high-priority
> queue just like other ETH_P_PAE frames.
>

> diff --git a/drivers/net/wireless/rsi/rsi_91x_core.c b/drivers/net/wireless/rsi/rsi_91x_core.c
> index 0f3a80f66b61c..d76c9dc99cafa 100644
> --- a/drivers/net/wireless/rsi/rsi_91x_core.c
> +++ b/drivers/net/wireless/rsi/rsi_91x_core.c
> +
> if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
> + tx_eapol = true;
> + } else if (skb->protocol == cpu_to_be16(ETH_P_802_3)) {
> + hdr_len = ieee80211_get_hdrlen_from_skb(skb) +
> + sizeof(rfc1042_header) - ETH_HLEN + 2;
> + eth_hdr = (struct ethhdr *)(skb->data + hdr_len);
> + if (eth_hdr->h_proto == cpu_to_be16(ETH_P_PAE))
> + tx_eapol = true;
> + }
> +
> diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
> index c61f83a7333b6..d43754fff287d 100644
> @@ -168,6 +171,16 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
> + if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
> + tx_eapol = true;
> + } else if (skb->protocol == cpu_to_be16(ETH_P_802_3)) {
> + hdr_len = ieee80211_get_hdrlen_from_skb(skb) +
> + sizeof(rfc1042_header) - ETH_HLEN + 2;
> + eth_hdr = (struct ethhdr *)(skb->data + hdr_len);
> + if (eth_hdr->h_proto == cpu_to_be16(ETH_P_PAE))
> + tx_eapol = true;
> + }
> +

It looks like the same logic is being duplicated twice. Maybe create a
helper function for it, something like bool rsi_is_eapol(struct
sk_buff *skb) ?

Also I think it would be good to tag this for stable.

Martin

2022-09-23 14:20:36

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] wifi: rsi: Fix handling of 802.3 EAPOL frames sent via control port

On 9/23/22 09:11, Fuzzey, Martin wrote:
> Hi Marek,

Hi,

> On Thu, 22 Sept 2022 at 22:33, Marek Vasut <[email protected]> wrote:
>>
>> When using wpa_supplicant v2.10, this driver is no longer able to
>> associate with any AP and fails in the EAPOL 4-way handshake while
>> sending the 2/4 message to the AP. The problem is not present in
>> wpa_supplicant v2.9 or older. The problem stems from HostAP commit
>> 144314eaa ("wpa_supplicant: Send EAPOL frames over nl80211 where available")
>> which changes the way EAPOL frames are sent, from them being send
>> at L2 frames to them being sent via nl80211 control port.
> ...
>> Therefore, to fix this problem, inspect the ETH_P_802_3 frames in
>> the rsi_91x driver, check the ethertype of the encapsulated frame,
>> and in case it is ETH_P_PAE, transmit the frame via high-priority
>> queue just like other ETH_P_PAE frames.
>>
>
>> diff --git a/drivers/net/wireless/rsi/rsi_91x_core.c b/drivers/net/wireless/rsi/rsi_91x_core.c
>> index 0f3a80f66b61c..d76c9dc99cafa 100644
>> --- a/drivers/net/wireless/rsi/rsi_91x_core.c
>> +++ b/drivers/net/wireless/rsi/rsi_91x_core.c
>> +
>> if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
>> + tx_eapol = true;
>> + } else if (skb->protocol == cpu_to_be16(ETH_P_802_3)) {
>> + hdr_len = ieee80211_get_hdrlen_from_skb(skb) +
>> + sizeof(rfc1042_header) - ETH_HLEN + 2;
>> + eth_hdr = (struct ethhdr *)(skb->data + hdr_len);
>> + if (eth_hdr->h_proto == cpu_to_be16(ETH_P_PAE))
>> + tx_eapol = true;
>> + }
>> +
>> diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
>> index c61f83a7333b6..d43754fff287d 100644
>> @@ -168,6 +171,16 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
>> + if (skb->protocol == cpu_to_be16(ETH_P_PAE)) {
>> + tx_eapol = true;
>> + } else if (skb->protocol == cpu_to_be16(ETH_P_802_3)) {
>> + hdr_len = ieee80211_get_hdrlen_from_skb(skb) +
>> + sizeof(rfc1042_header) - ETH_HLEN + 2;
>> + eth_hdr = (struct ethhdr *)(skb->data + hdr_len);
>> + if (eth_hdr->h_proto == cpu_to_be16(ETH_P_PAE))
>> + tx_eapol = true;
>> + }
>> +
>
> It looks like the same logic is being duplicated twice. Maybe create a
> helper function for it, something like bool rsi_is_eapol(struct
> sk_buff *skb) ?

Sure, I'll do that in V2.

Currently I am looking for input whether this reaching into an 8023
packet to see if it is EAPOL one is even the right approach:
"
NOTE: I am really unsure about the method of finding out the exact
place of ethernet header in the encapsulated packet and then
extracting the ethertype from it. Is there maybe some sort of
helper function for that purpose ?
"

> Also I think it would be good to tag this for stable.

There is a Fixes tag:
"
Fixes: 0eb42586cf876 ("rsi: data packet descriptor enhancements")
"