All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] wifi: wilc1000: fix RCU warning in connect path
@ 2024-01-04 14:39 Alexis Lothoré
  2024-01-04 14:39 ` [PATCH 1/2] wifi: wilc1000: fix declarations ordering Alexis Lothoré
  2024-01-04 14:39 ` [PATCH 2/2] wifi: wilc1000: fix RCU usage in connect path Alexis Lothoré
  0 siblings, 2 replies; 5+ messages in thread
From: Alexis Lothoré @ 2024-01-04 14:39 UTC (permalink / raw)
  To: linux-wireless
  Cc: Ajay Singh, Claudiu Beznea, Kalle Valo, Thomas Petazzoni, linux-kernel

Hello,
this small series aims to fix a RCU warning due to RCU pointer dereference
outside of a RCU critical read section.

Fix is in the second commit, the first one is a mere declarations
reordering before touching declared variables in wilc_parse_join_bss_param

Alexis Lothoré (2):
  wifi: wilc1000: fix declarations ordering
  wifi: wilc1000: fix RCU usage in connect path

 drivers/net/wireless/microchip/wilc1000/hif.c | 39 ++++++++++++-------
 1 file changed, 25 insertions(+), 14 deletions(-)

-- 
2.42.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] wifi: wilc1000: fix declarations ordering
  2024-01-04 14:39 [PATCH 0/2] wifi: wilc1000: fix RCU warning in connect path Alexis Lothoré
@ 2024-01-04 14:39 ` Alexis Lothoré
  2024-01-04 14:39 ` [PATCH 2/2] wifi: wilc1000: fix RCU usage in connect path Alexis Lothoré
  1 sibling, 0 replies; 5+ messages in thread
From: Alexis Lothoré @ 2024-01-04 14:39 UTC (permalink / raw)
  To: linux-wireless
  Cc: Ajay Singh, Claudiu Beznea, Kalle Valo, Thomas Petazzoni, linux-kernel

From: Alexis Lothoré <alexis.lothore@bootlin.com>

Reorder parameters declaration in wilc_parse_join_bss_param to enforce
reverse christmas tree

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/hif.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index 839f142663e8..6f1218a51061 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -377,13 +377,13 @@ struct wilc_join_bss_param *
 wilc_parse_join_bss_param(struct cfg80211_bss *bss,
 			  struct cfg80211_crypto_settings *crypto)
 {
-	struct wilc_join_bss_param *param;
-	struct ieee80211_p2p_noa_attr noa_attr;
-	u8 rates_len = 0;
+	const struct cfg80211_bss_ies *ies = rcu_dereference(bss->ies);
 	const u8 *tim_elm, *ssid_elm, *rates_ie, *supp_rates_ie;
 	const u8 *ht_ie, *wpa_ie, *wmm_ie, *rsn_ie;
+	struct ieee80211_p2p_noa_attr noa_attr;
+	struct wilc_join_bss_param *param;
+	u8 rates_len = 0;
 	int ret;
-	const struct cfg80211_bss_ies *ies = rcu_dereference(bss->ies);
 
 	param = kzalloc(sizeof(*param), GFP_KERNEL);
 	if (!param)
-- 
2.42.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] wifi: wilc1000: fix RCU usage in connect path
  2024-01-04 14:39 [PATCH 0/2] wifi: wilc1000: fix RCU warning in connect path Alexis Lothoré
  2024-01-04 14:39 ` [PATCH 1/2] wifi: wilc1000: fix declarations ordering Alexis Lothoré
@ 2024-01-04 14:39 ` Alexis Lothoré
  2024-01-04 15:30   ` Christophe JAILLET
  1 sibling, 1 reply; 5+ messages in thread
From: Alexis Lothoré @ 2024-01-04 14:39 UTC (permalink / raw)
  To: linux-wireless
  Cc: Ajay Singh, Claudiu Beznea, Kalle Valo, Thomas Petazzoni, linux-kernel

From: Alexis Lothoré <alexis.lothore@bootlin.com>

With lockdep enabled, calls to the connect function from cfg802.11 layer
lead to the following warning:

=============================
WARNING: suspicious RCU usage
6.7.0-rc1-wt+ #333 Not tainted
-----------------------------
drivers/net/wireless/microchip/wilc1000/hif.c:386
suspicious rcu_dereference_check() usage!
[...]
stack backtrace:
CPU: 0 PID: 100 Comm: wpa_supplicant Not tainted 6.7.0-rc1-wt+ #333
Hardware name: Atmel SAMA5
 unwind_backtrace from show_stack+0x18/0x1c
 show_stack from dump_stack_lvl+0x34/0x48
 dump_stack_lvl from wilc_parse_join_bss_param+0x7dc/0x7f4
 wilc_parse_join_bss_param from connect+0x2c4/0x648
 connect from cfg80211_connect+0x30c/0xb74
 cfg80211_connect from nl80211_connect+0x860/0xa94
 nl80211_connect from genl_rcv_msg+0x3fc/0x59c
 genl_rcv_msg from netlink_rcv_skb+0xd0/0x1f8
 netlink_rcv_skb from genl_rcv+0x2c/0x3c
 genl_rcv from netlink_unicast+0x3b0/0x550
 netlink_unicast from netlink_sendmsg+0x368/0x688
 netlink_sendmsg from ____sys_sendmsg+0x190/0x430
 ____sys_sendmsg from ___sys_sendmsg+0x110/0x158
 ___sys_sendmsg from sys_sendmsg+0xe8/0x150
 sys_sendmsg from ret_fast_syscall+0x0/0x1c

This warning is emitted because in the connect path, when trying to parse
target BSS parameters, we dereference a RCU pointer whithout being in RCU
critical section.
Fix RCU dereference usage by moving it to a RCU read critical section. To
avoid wrapping the whole wilc_parse_join_bss_param under the critical
section, just use the critical section to copy ies data

Fixes: c460495ee072 ("staging: wilc1000: fix incorrent type in initializer")
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/hif.c | 35 ++++++++++++-------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index 6f1218a51061..806b7337b5ae 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -377,38 +377,48 @@ struct wilc_join_bss_param *
 wilc_parse_join_bss_param(struct cfg80211_bss *bss,
 			  struct cfg80211_crypto_settings *crypto)
 {
-	const struct cfg80211_bss_ies *ies = rcu_dereference(bss->ies);
-	const u8 *tim_elm, *ssid_elm, *rates_ie, *supp_rates_ie;
+	const u8 *ies_data, *tim_elm, *ssid_elm, *rates_ie, *supp_rates_ie;
 	const u8 *ht_ie, *wpa_ie, *wmm_ie, *rsn_ie;
 	struct ieee80211_p2p_noa_attr noa_attr;
+	const struct cfg80211_bss_ies *ies;
 	struct wilc_join_bss_param *param;
-	u8 rates_len = 0;
+	u8 rates_len = 0, ies_len;
 	int ret;
 
 	param = kzalloc(sizeof(*param), GFP_KERNEL);
 	if (!param)
 		return NULL;
 
+	rcu_read_lock();
+	ies = rcu_dereference(bss->ies);
+	ies_data = kmemdup(ies->data, ies->len, GFP_ATOMIC);
+	if (!ies_data) {
+		rcu_read_unlock();
+		return NULL;
+	}
+	ies_len = ies->len;
+	rcu_read_unlock();
+
 	param->beacon_period = cpu_to_le16(bss->beacon_interval);
 	param->cap_info = cpu_to_le16(bss->capability);
 	param->bss_type = WILC_FW_BSS_TYPE_INFRA;
 	param->ch = ieee80211_frequency_to_channel(bss->channel->center_freq);
 	ether_addr_copy(param->bssid, bss->bssid);
 
-	ssid_elm = cfg80211_find_ie(WLAN_EID_SSID, ies->data, ies->len);
+	ssid_elm = cfg80211_find_ie(WLAN_EID_SSID, ies_data, ies_len);
 	if (ssid_elm) {
 		if (ssid_elm[1] <= IEEE80211_MAX_SSID_LEN)
 			memcpy(param->ssid, ssid_elm + 2, ssid_elm[1]);
 	}
 
-	tim_elm = cfg80211_find_ie(WLAN_EID_TIM, ies->data, ies->len);
+	tim_elm = cfg80211_find_ie(WLAN_EID_TIM, ies_data, ies_len);
 	if (tim_elm && tim_elm[1] >= 2)
 		param->dtim_period = tim_elm[3];
 
 	memset(param->p_suites, 0xFF, 3);
 	memset(param->akm_suites, 0xFF, 3);
 
-	rates_ie = cfg80211_find_ie(WLAN_EID_SUPP_RATES, ies->data, ies->len);
+	rates_ie = cfg80211_find_ie(WLAN_EID_SUPP_RATES, ies_data, ies_len);
 	if (rates_ie) {
 		rates_len = rates_ie[1];
 		if (rates_len > WILC_MAX_RATES_SUPPORTED)
@@ -419,7 +429,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
 
 	if (rates_len < WILC_MAX_RATES_SUPPORTED) {
 		supp_rates_ie = cfg80211_find_ie(WLAN_EID_EXT_SUPP_RATES,
-						 ies->data, ies->len);
+						 ies_data, ies_len);
 		if (supp_rates_ie) {
 			u8 ext_rates = supp_rates_ie[1];
 
@@ -434,11 +444,11 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
 		}
 	}
 
-	ht_ie = cfg80211_find_ie(WLAN_EID_HT_CAPABILITY, ies->data, ies->len);
+	ht_ie = cfg80211_find_ie(WLAN_EID_HT_CAPABILITY, ies_data, ies_len);
 	if (ht_ie)
 		param->ht_capable = true;
 
-	ret = cfg80211_get_p2p_attr(ies->data, ies->len,
+	ret = cfg80211_get_p2p_attr(ies_data, ies_len,
 				    IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
 				    (u8 *)&noa_attr, sizeof(noa_attr));
 	if (ret > 0) {
@@ -462,7 +472,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
 	}
 	wmm_ie = cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
 					 WLAN_OUI_TYPE_MICROSOFT_WMM,
-					 ies->data, ies->len);
+					 ies_data, ies_len);
 	if (wmm_ie) {
 		struct ieee80211_wmm_param_ie *ie;
 
@@ -477,13 +487,13 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
 
 	wpa_ie = cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
 					 WLAN_OUI_TYPE_MICROSOFT_WPA,
-					 ies->data, ies->len);
+					 ies_data, ies_len);
 	if (wpa_ie) {
 		param->mode_802_11i = 1;
 		param->rsn_found = true;
 	}
 
-	rsn_ie = cfg80211_find_ie(WLAN_EID_RSN, ies->data, ies->len);
+	rsn_ie = cfg80211_find_ie(WLAN_EID_RSN, ies_data, ies_len);
 	if (rsn_ie) {
 		int rsn_ie_len = sizeof(struct element) + rsn_ie[1];
 		int offset = 8;
@@ -517,6 +527,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
 			param->akm_suites[i] = crypto->akm_suites[i] & 0xFF;
 	}
 
+	kfree(ies_data);
 	return (void *)param;
 }
 
-- 
2.42.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] wifi: wilc1000: fix RCU usage in connect path
  2024-01-04 14:39 ` [PATCH 2/2] wifi: wilc1000: fix RCU usage in connect path Alexis Lothoré
@ 2024-01-04 15:30   ` Christophe JAILLET
  2024-01-04 15:41     ` Alexis Lothoré
  0 siblings, 1 reply; 5+ messages in thread
From: Christophe JAILLET @ 2024-01-04 15:30 UTC (permalink / raw)
  To: alexis.lothore
  Cc: ajay.kathat, claudiu.beznea, kvalo, linux-kernel, linux-wireless,
	thomas.petazzoni

Le 04/01/2024 à 15:39, Alexis Lothoré a écrit :
> From: Alexis Lothoré <alexis.lothore-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> 
> With lockdep enabled, calls to the connect function from cfg802.11 layer
> lead to the following warning:
> 
> =============================
> WARNING: suspicious RCU usage
> 6.7.0-rc1-wt+ #333 Not tainted
> -----------------------------
> drivers/net/wireless/microchip/wilc1000/hif.c:386
> suspicious rcu_dereference_check() usage!
> [...]
> stack backtrace:
> CPU: 0 PID: 100 Comm: wpa_supplicant Not tainted 6.7.0-rc1-wt+ #333
> Hardware name: Atmel SAMA5
>   unwind_backtrace from show_stack+0x18/0x1c
>   show_stack from dump_stack_lvl+0x34/0x48
>   dump_stack_lvl from wilc_parse_join_bss_param+0x7dc/0x7f4
>   wilc_parse_join_bss_param from connect+0x2c4/0x648
>   connect from cfg80211_connect+0x30c/0xb74
>   cfg80211_connect from nl80211_connect+0x860/0xa94
>   nl80211_connect from genl_rcv_msg+0x3fc/0x59c
>   genl_rcv_msg from netlink_rcv_skb+0xd0/0x1f8
>   netlink_rcv_skb from genl_rcv+0x2c/0x3c
>   genl_rcv from netlink_unicast+0x3b0/0x550
>   netlink_unicast from netlink_sendmsg+0x368/0x688
>   netlink_sendmsg from ____sys_sendmsg+0x190/0x430
>   ____sys_sendmsg from ___sys_sendmsg+0x110/0x158
>   ___sys_sendmsg from sys_sendmsg+0xe8/0x150
>   sys_sendmsg from ret_fast_syscall+0x0/0x1c
> 
> This warning is emitted because in the connect path, when trying to parse
> target BSS parameters, we dereference a RCU pointer whithout being in RCU
> critical section.
> Fix RCU dereference usage by moving it to a RCU read critical section. To
> avoid wrapping the whole wilc_parse_join_bss_param under the critical
> section, just use the critical section to copy ies data
> 
> Fixes: c460495ee072 ("staging: wilc1000: fix incorrent type in initializer")
> Signed-off-by: Alexis Lothoré <alexis.lothore-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> ---
>   drivers/net/wireless/microchip/wilc1000/hif.c | 35 ++++++++++++-------
>   1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
> index 6f1218a51061..806b7337b5ae 100644
> --- a/drivers/net/wireless/microchip/wilc1000/hif.c
> +++ b/drivers/net/wireless/microchip/wilc1000/hif.c
> @@ -377,38 +377,48 @@ struct wilc_join_bss_param *
>   wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>   			  struct cfg80211_crypto_settings *crypto)
>   {
> -	const struct cfg80211_bss_ies *ies = rcu_dereference(bss->ies);
> -	const u8 *tim_elm, *ssid_elm, *rates_ie, *supp_rates_ie;
> +	const u8 *ies_data, *tim_elm, *ssid_elm, *rates_ie, *supp_rates_ie;
>   	const u8 *ht_ie, *wpa_ie, *wmm_ie, *rsn_ie;
>   	struct ieee80211_p2p_noa_attr noa_attr;
> +	const struct cfg80211_bss_ies *ies;
>   	struct wilc_join_bss_param *param;
> -	u8 rates_len = 0;
> +	u8 rates_len = 0, ies_len;
>   	int ret;
>   
>   	param = kzalloc(sizeof(*param), GFP_KERNEL);
>   	if (!param)
>   		return NULL;
>   
> +	rcu_read_lock();
> +	ies = rcu_dereference(bss->ies);
> +	ies_data = kmemdup(ies->data, ies->len, GFP_ATOMIC);

Is it mandatory to have GFP_ATOMIC here?
If yes, then some other drivers may need to be fixed (at least [1]).

> +	if (!ies_data) {
> +		rcu_read_unlock();

'param' allocated just a few lines above is leaking now.

CJ


[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/p2pdma.c#n660


> +		return NULL;
> +	}
> +	ies_len = ies->len;
> +	rcu_read_unlock();
> +
>   	param->beacon_period = cpu_to_le16(bss->beacon_interval);
>   	param->cap_info = cpu_to_le16(bss->capability);
>   	param->bss_type = WILC_FW_BSS_TYPE_INFRA;
>   	param->ch = ieee80211_frequency_to_channel(bss->channel->center_freq);
>   	ether_addr_copy(param->bssid, bss->bssid);
>   
> -	ssid_elm = cfg80211_find_ie(WLAN_EID_SSID, ies->data, ies->len);
> +	ssid_elm = cfg80211_find_ie(WLAN_EID_SSID, ies_data, ies_len);
>   	if (ssid_elm) {
>   		if (ssid_elm[1] <= IEEE80211_MAX_SSID_LEN)
>   			memcpy(param->ssid, ssid_elm + 2, ssid_elm[1]);
>   	}
>   
> -	tim_elm = cfg80211_find_ie(WLAN_EID_TIM, ies->data, ies->len);
> +	tim_elm = cfg80211_find_ie(WLAN_EID_TIM, ies_data, ies_len);
>   	if (tim_elm && tim_elm[1] >= 2)
>   		param->dtim_period = tim_elm[3];
>   
>   	memset(param->p_suites, 0xFF, 3);
>   	memset(param->akm_suites, 0xFF, 3);
>   
> -	rates_ie = cfg80211_find_ie(WLAN_EID_SUPP_RATES, ies->data, ies->len);
> +	rates_ie = cfg80211_find_ie(WLAN_EID_SUPP_RATES, ies_data, ies_len);
>   	if (rates_ie) {
>   		rates_len = rates_ie[1];
>   		if (rates_len > WILC_MAX_RATES_SUPPORTED)
> @@ -419,7 +429,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>   
>   	if (rates_len < WILC_MAX_RATES_SUPPORTED) {
>   		supp_rates_ie = cfg80211_find_ie(WLAN_EID_EXT_SUPP_RATES,
> -						 ies->data, ies->len);
> +						 ies_data, ies_len);
>   		if (supp_rates_ie) {
>   			u8 ext_rates = supp_rates_ie[1];
>   
> @@ -434,11 +444,11 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>   		}
>   	}
>   
> -	ht_ie = cfg80211_find_ie(WLAN_EID_HT_CAPABILITY, ies->data, ies->len);
> +	ht_ie = cfg80211_find_ie(WLAN_EID_HT_CAPABILITY, ies_data, ies_len);
>   	if (ht_ie)
>   		param->ht_capable = true;
>   
> -	ret = cfg80211_get_p2p_attr(ies->data, ies->len,
> +	ret = cfg80211_get_p2p_attr(ies_data, ies_len,
>   				    IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
>   				    (u8 *)&noa_attr, sizeof(noa_attr));
>   	if (ret > 0) {
> @@ -462,7 +472,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>   	}
>   	wmm_ie = cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
>   					 WLAN_OUI_TYPE_MICROSOFT_WMM,
> -					 ies->data, ies->len);
> +					 ies_data, ies_len);
>   	if (wmm_ie) {
>   		struct ieee80211_wmm_param_ie *ie;
>   
> @@ -477,13 +487,13 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>   
>   	wpa_ie = cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
>   					 WLAN_OUI_TYPE_MICROSOFT_WPA,
> -					 ies->data, ies->len);
> +					 ies_data, ies_len);
>   	if (wpa_ie) {
>   		param->mode_802_11i = 1;
>   		param->rsn_found = true;
>   	}
>   
> -	rsn_ie = cfg80211_find_ie(WLAN_EID_RSN, ies->data, ies->len);
> +	rsn_ie = cfg80211_find_ie(WLAN_EID_RSN, ies_data, ies_len);
>   	if (rsn_ie) {
>   		int rsn_ie_len = sizeof(struct element) + rsn_ie[1];
>   		int offset = 8;
> @@ -517,6 +527,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>   			param->akm_suites[i] = crypto->akm_suites[i] & 0xFF;
>   	}
>   
> +	kfree(ies_data);
>   	return (void *)param;
>   }
>   


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] wifi: wilc1000: fix RCU usage in connect path
  2024-01-04 15:30   ` Christophe JAILLET
@ 2024-01-04 15:41     ` Alexis Lothoré
  0 siblings, 0 replies; 5+ messages in thread
From: Alexis Lothoré @ 2024-01-04 15:41 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: ajay.kathat, claudiu.beznea, kvalo, linux-kernel, linux-wireless,
	thomas.petazzoni

On 1/4/24 16:30, Christophe JAILLET wrote:
> Le 04/01/2024 à 15:39, Alexis Lothoré a écrit :
>> From: Alexis Lothoré <alexis.lothore-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
>>
>> With lockdep enabled, calls to the connect function from cfg802.11 layer
>> lead to the following warning:
>>
>> =============================
>> WARNING: suspicious RCU usage
>> 6.7.0-rc1-wt+ #333 Not tainted
>> -----------------------------
>> drivers/net/wireless/microchip/wilc1000/hif.c:386
>> suspicious rcu_dereference_check() usage!
>> [...]
>> stack backtrace:
>> CPU: 0 PID: 100 Comm: wpa_supplicant Not tainted 6.7.0-rc1-wt+ #333
>> Hardware name: Atmel SAMA5
>>   unwind_backtrace from show_stack+0x18/0x1c
>>   show_stack from dump_stack_lvl+0x34/0x48
>>   dump_stack_lvl from wilc_parse_join_bss_param+0x7dc/0x7f4
>>   wilc_parse_join_bss_param from connect+0x2c4/0x648
>>   connect from cfg80211_connect+0x30c/0xb74
>>   cfg80211_connect from nl80211_connect+0x860/0xa94
>>   nl80211_connect from genl_rcv_msg+0x3fc/0x59c
>>   genl_rcv_msg from netlink_rcv_skb+0xd0/0x1f8
>>   netlink_rcv_skb from genl_rcv+0x2c/0x3c
>>   genl_rcv from netlink_unicast+0x3b0/0x550
>>   netlink_unicast from netlink_sendmsg+0x368/0x688
>>   netlink_sendmsg from ____sys_sendmsg+0x190/0x430
>>   ____sys_sendmsg from ___sys_sendmsg+0x110/0x158
>>   ___sys_sendmsg from sys_sendmsg+0xe8/0x150
>>   sys_sendmsg from ret_fast_syscall+0x0/0x1c
>>
>> This warning is emitted because in the connect path, when trying to parse
>> target BSS parameters, we dereference a RCU pointer whithout being in RCU
>> critical section.
>> Fix RCU dereference usage by moving it to a RCU read critical section. To
>> avoid wrapping the whole wilc_parse_join_bss_param under the critical
>> section, just use the critical section to copy ies data
>>
>> Fixes: c460495ee072 ("staging: wilc1000: fix incorrent type in initializer")
>> Signed-off-by: Alexis Lothoré
>> <alexis.lothore-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/net/wireless/microchip/wilc1000/hif.c | 35 ++++++++++++-------
>>   1 file changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c
>> b/drivers/net/wireless/microchip/wilc1000/hif.c
>> index 6f1218a51061..806b7337b5ae 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/hif.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/hif.c
>> @@ -377,38 +377,48 @@ struct wilc_join_bss_param *
>>   wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>>                 struct cfg80211_crypto_settings *crypto)
>>   {
>> -    const struct cfg80211_bss_ies *ies = rcu_dereference(bss->ies);
>> -    const u8 *tim_elm, *ssid_elm, *rates_ie, *supp_rates_ie;
>> +    const u8 *ies_data, *tim_elm, *ssid_elm, *rates_ie, *supp_rates_ie;
>>       const u8 *ht_ie, *wpa_ie, *wmm_ie, *rsn_ie;
>>       struct ieee80211_p2p_noa_attr noa_attr;
>> +    const struct cfg80211_bss_ies *ies;
>>       struct wilc_join_bss_param *param;
>> -    u8 rates_len = 0;
>> +    u8 rates_len = 0, ies_len;
>>       int ret;
>>         param = kzalloc(sizeof(*param), GFP_KERNEL);
>>       if (!param)
>>           return NULL;
>>   +    rcu_read_lock();
>> +    ies = rcu_dereference(bss->ies);
>> +    ies_data = kmemdup(ies->data, ies->len, GFP_ATOMIC);
> 
> Is it mandatory to have GFP_ATOMIC here?
> If yes, then some other drivers may need to be fixed (at least [1]).

From my understanding: yes, because if any sleep occurs in a section protected
by rcu_read_lock/rcu_read_unlock, it may mess with RCU grace period wait (and
so, rcu reclaim step may occur while this reader is still in the read section)

> 
>> +    if (!ies_data) {
>> +        rcu_read_unlock();
> 
> 'param' allocated just a few lines above is leaking now.

You are right, good catch, thanks. I will update accordingly.
> 
> CJ
> 
> 
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/pci/p2pdma.c#n660
> 
> 
>> +        return NULL;
>> +    }
>> +    ies_len = ies->len;
>> +    rcu_read_unlock();
>> +
>>       param->beacon_period = cpu_to_le16(bss->beacon_interval);
>>       param->cap_info = cpu_to_le16(bss->capability);
>>       param->bss_type = WILC_FW_BSS_TYPE_INFRA;
>>       param->ch = ieee80211_frequency_to_channel(bss->channel->center_freq);
>>       ether_addr_copy(param->bssid, bss->bssid);
>>   -    ssid_elm = cfg80211_find_ie(WLAN_EID_SSID, ies->data, ies->len);
>> +    ssid_elm = cfg80211_find_ie(WLAN_EID_SSID, ies_data, ies_len);
>>       if (ssid_elm) {
>>           if (ssid_elm[1] <= IEEE80211_MAX_SSID_LEN)
>>               memcpy(param->ssid, ssid_elm + 2, ssid_elm[1]);
>>       }
>>   -    tim_elm = cfg80211_find_ie(WLAN_EID_TIM, ies->data, ies->len);
>> +    tim_elm = cfg80211_find_ie(WLAN_EID_TIM, ies_data, ies_len);
>>       if (tim_elm && tim_elm[1] >= 2)
>>           param->dtim_period = tim_elm[3];
>>         memset(param->p_suites, 0xFF, 3);
>>       memset(param->akm_suites, 0xFF, 3);
>>   -    rates_ie = cfg80211_find_ie(WLAN_EID_SUPP_RATES, ies->data, ies->len);
>> +    rates_ie = cfg80211_find_ie(WLAN_EID_SUPP_RATES, ies_data, ies_len);
>>       if (rates_ie) {
>>           rates_len = rates_ie[1];
>>           if (rates_len > WILC_MAX_RATES_SUPPORTED)
>> @@ -419,7 +429,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>>         if (rates_len < WILC_MAX_RATES_SUPPORTED) {
>>           supp_rates_ie = cfg80211_find_ie(WLAN_EID_EXT_SUPP_RATES,
>> -                         ies->data, ies->len);
>> +                         ies_data, ies_len);
>>           if (supp_rates_ie) {
>>               u8 ext_rates = supp_rates_ie[1];
>>   @@ -434,11 +444,11 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>>           }
>>       }
>>   -    ht_ie = cfg80211_find_ie(WLAN_EID_HT_CAPABILITY, ies->data, ies->len);
>> +    ht_ie = cfg80211_find_ie(WLAN_EID_HT_CAPABILITY, ies_data, ies_len);
>>       if (ht_ie)
>>           param->ht_capable = true;
>>   -    ret = cfg80211_get_p2p_attr(ies->data, ies->len,
>> +    ret = cfg80211_get_p2p_attr(ies_data, ies_len,
>>                       IEEE80211_P2P_ATTR_ABSENCE_NOTICE,
>>                       (u8 *)&noa_attr, sizeof(noa_attr));
>>       if (ret > 0) {
>> @@ -462,7 +472,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>>       }
>>       wmm_ie = cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
>>                        WLAN_OUI_TYPE_MICROSOFT_WMM,
>> -                     ies->data, ies->len);
>> +                     ies_data, ies_len);
>>       if (wmm_ie) {
>>           struct ieee80211_wmm_param_ie *ie;
>>   @@ -477,13 +487,13 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>>         wpa_ie = cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
>>                        WLAN_OUI_TYPE_MICROSOFT_WPA,
>> -                     ies->data, ies->len);
>> +                     ies_data, ies_len);
>>       if (wpa_ie) {
>>           param->mode_802_11i = 1;
>>           param->rsn_found = true;
>>       }
>>   -    rsn_ie = cfg80211_find_ie(WLAN_EID_RSN, ies->data, ies->len);
>> +    rsn_ie = cfg80211_find_ie(WLAN_EID_RSN, ies_data, ies_len);
>>       if (rsn_ie) {
>>           int rsn_ie_len = sizeof(struct element) + rsn_ie[1];
>>           int offset = 8;
>> @@ -517,6 +527,7 @@ wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>>               param->akm_suites[i] = crypto->akm_suites[i] & 0xFF;
>>       }
>>   +    kfree(ies_data);
>>       return (void *)param;
>>   }
>>   
> 

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-01-04 15:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-04 14:39 [PATCH 0/2] wifi: wilc1000: fix RCU warning in connect path Alexis Lothoré
2024-01-04 14:39 ` [PATCH 1/2] wifi: wilc1000: fix declarations ordering Alexis Lothoré
2024-01-04 14:39 ` [PATCH 2/2] wifi: wilc1000: fix RCU usage in connect path Alexis Lothoré
2024-01-04 15:30   ` Christophe JAILLET
2024-01-04 15:41     ` Alexis Lothoré

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.