diff mbox series

[1/2] wifi: cfg80211: Reject (re-)association to the same BSSID

Message ID 20230602225751.164525-1-kglund@google.com (mailing list archive)
State Rejected
Delegated to: Johannes Berg
Headers show
Series [1/2] wifi: cfg80211: Reject (re-)association to the same BSSID | expand

Commit Message

Kevin Lund June 2, 2023, 10:57 p.m. UTC 
Within cfg80211_connect, reject the (re-)association request if we are
already connected to the exact BSSID which is being requested. This
prevents an unnecessary attempt to connect which in the best case
leaves us back where we started.

There is precedent for behaving this way over on the userspace SME side
of things in cfg80211_mlme_auth. Further, cfg80211_connect already makes
several basic checks to ensure the connection attempt is reasonable, so
this fits in that context.

Signed-off-by: Kevin Lund <kglund@google.com>
---
 net/wireless/sme.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

kernel test robot June 3, 2023, 3:32 a.m. UTC | #1 
Hi Kevin,

kernel test robot noticed the following build errors:

[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kevin-Lund/wifi-mwifiex-Stop-rejecting-connection-attempts-while-connected/20230603-065907
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link:    https://lore.kernel.org/r/20230602225751.164525-1-kglund%40google.com
patch subject: [PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID
config: arm-randconfig-r046-20230602 (https://download.01.org/0day-ci/archive/20230603/202306031152.pDkq23ib-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/09f9fe87fe3588d03dafcaf05b36b3e931f8c8eb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kevin-Lund/wifi-mwifiex-Stop-rejecting-connection-attempts-while-connected/20230603-065907
        git checkout 09f9fe87fe3588d03dafcaf05b36b3e931f8c8eb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash net/wireless/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306031152.pDkq23ib-lkp@intel.com/

All errors (new ones prefixed by >>):

   net/wireless/sme.c: In function 'cfg80211_connect':
>> net/wireless/sme.c:1454:42: error: 'struct wireless_dev' has no member named 'current_bss'
    1454 |                 if (ether_addr_equal(wdev->current_bss->pub.bssid,
         |                                          ^~


vim +1454 net/wireless/sme.c

  1418	
  1419	/*
  1420	 * API calls for nl80211/wext compatibility code
  1421	 */
  1422	int cfg80211_connect(struct cfg80211_registered_device *rdev,
  1423			     struct net_device *dev,
  1424			     struct cfg80211_connect_params *connect,
  1425			     struct cfg80211_cached_keys *connkeys,
  1426			     const u8 *prev_bssid)
  1427	{
  1428		struct wireless_dev *wdev = dev->ieee80211_ptr;
  1429		int err;
  1430	
  1431		ASSERT_WDEV_LOCK(wdev);
  1432	
  1433		/*
  1434		 * If we have an ssid_len, we're trying to connect or are
  1435		 * already connected, so reject a new SSID unless it's the
  1436		 * same (which is the case for re-association.)
  1437		 */
  1438		if (wdev->u.client.ssid_len &&
  1439		    (wdev->u.client.ssid_len != connect->ssid_len ||
  1440		     memcmp(wdev->u.client.ssid, connect->ssid, wdev->u.client.ssid_len)))
  1441			return -EALREADY;
  1442	
  1443		/*
  1444		 * If connected, reject (re-)association unless prev_bssid
  1445		 * matches the current BSSID. Also reject if the current BSSID matches
  1446		 * the desired BSSID.
  1447		 */
  1448		if (wdev->connected) {
  1449			if (!prev_bssid)
  1450				return -EALREADY;
  1451			if (!ether_addr_equal(prev_bssid,
  1452					      wdev->u.client.connected_addr))
  1453				return -ENOTCONN;
> 1454			if (ether_addr_equal(wdev->current_bss->pub.bssid,
  1455					     connect->bssid))
  1456				return -EALREADY;
  1457		}
  1458	
  1459		/*
  1460		 * Reject if we're in the process of connecting with WEP,
  1461		 * this case isn't very interesting and trying to handle
  1462		 * it would make the code much more complex.
  1463		 */
  1464		if (wdev->connect_keys)
  1465			return -EINPROGRESS;
  1466	
  1467		cfg80211_oper_and_ht_capa(&connect->ht_capa_mask,
  1468					  rdev->wiphy.ht_capa_mod_mask);
  1469		cfg80211_oper_and_vht_capa(&connect->vht_capa_mask,
  1470					   rdev->wiphy.vht_capa_mod_mask);
  1471	
  1472		if (connkeys && connkeys->def >= 0) {
  1473			int idx;
  1474			u32 cipher;
  1475	
  1476			idx = connkeys->def;
  1477			cipher = connkeys->params[idx].cipher;
  1478			/* If given a WEP key we may need it for shared key auth */
  1479			if (cipher == WLAN_CIPHER_SUITE_WEP40 ||
  1480			    cipher == WLAN_CIPHER_SUITE_WEP104) {
  1481				connect->key_idx = idx;
  1482				connect->key = connkeys->params[idx].key;
  1483				connect->key_len = connkeys->params[idx].key_len;
  1484	
  1485				/*
  1486				 * If ciphers are not set (e.g. when going through
  1487				 * iwconfig), we have to set them appropriately here.
  1488				 */
  1489				if (connect->crypto.cipher_group == 0)
  1490					connect->crypto.cipher_group = cipher;
  1491	
  1492				if (connect->crypto.n_ciphers_pairwise == 0) {
  1493					connect->crypto.n_ciphers_pairwise = 1;
  1494					connect->crypto.ciphers_pairwise[0] = cipher;
  1495				}
  1496			}
  1497		} else {
  1498			if (WARN_ON(connkeys))
  1499				return -EINVAL;
  1500	
  1501			/* connect can point to wdev->wext.connect which
  1502			 * can hold key data from a previous connection
  1503			 */
  1504			connect->key = NULL;
  1505			connect->key_len = 0;
  1506			connect->key_idx = 0;
  1507		}
  1508	
  1509		wdev->connect_keys = connkeys;
  1510		memcpy(wdev->u.client.ssid, connect->ssid, connect->ssid_len);
  1511		wdev->u.client.ssid_len = connect->ssid_len;
  1512	
  1513		wdev->conn_bss_type = connect->pbss ? IEEE80211_BSS_TYPE_PBSS :
  1514						      IEEE80211_BSS_TYPE_ESS;
  1515	
  1516		if (!rdev->ops->connect)
  1517			err = cfg80211_sme_connect(wdev, connect, prev_bssid);
  1518		else
  1519			err = rdev_connect(rdev, dev, connect);
  1520	
  1521		if (err) {
  1522			wdev->connect_keys = NULL;
  1523			/*
  1524			 * This could be reassoc getting refused, don't clear
  1525			 * ssid_len in that case.
  1526			 */
  1527			if (!wdev->connected)
  1528				wdev->u.client.ssid_len = 0;
  1529			return err;
  1530		}
  1531	
  1532		return 0;
  1533	}
  1534
kernel test robot June 3, 2023, 5:55 a.m. UTC | #2 
Hi Kevin,

kernel test robot noticed the following build errors:

[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kevin-Lund/wifi-mwifiex-Stop-rejecting-connection-attempts-while-connected/20230603-065907
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link:    https://lore.kernel.org/r/20230602225751.164525-1-kglund%40google.com
patch subject: [PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID
config: mips-ath79_defconfig (https://download.01.org/0day-ci/archive/20230603/202306031354.S6fuhpkH-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 4faf3aaf28226a4e950c103a14f6fc1d1fdabb1b)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/09f9fe87fe3588d03dafcaf05b36b3e931f8c8eb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kevin-Lund/wifi-mwifiex-Stop-rejecting-connection-attempts-while-connected/20230603-065907
        git checkout 09f9fe87fe3588d03dafcaf05b36b3e931f8c8eb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash net/wireless/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306031354.S6fuhpkH-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/wireless/sme.c:1454:30: error: no member named 'current_bss' in 'struct wireless_dev'
                   if (ether_addr_equal(wdev->current_bss->pub.bssid,
                                        ~~~~  ^
   1 error generated.


vim +1454 net/wireless/sme.c

  1418	
  1419	/*
  1420	 * API calls for nl80211/wext compatibility code
  1421	 */
  1422	int cfg80211_connect(struct cfg80211_registered_device *rdev,
  1423			     struct net_device *dev,
  1424			     struct cfg80211_connect_params *connect,
  1425			     struct cfg80211_cached_keys *connkeys,
  1426			     const u8 *prev_bssid)
  1427	{
  1428		struct wireless_dev *wdev = dev->ieee80211_ptr;
  1429		int err;
  1430	
  1431		ASSERT_WDEV_LOCK(wdev);
  1432	
  1433		/*
  1434		 * If we have an ssid_len, we're trying to connect or are
  1435		 * already connected, so reject a new SSID unless it's the
  1436		 * same (which is the case for re-association.)
  1437		 */
  1438		if (wdev->u.client.ssid_len &&
  1439		    (wdev->u.client.ssid_len != connect->ssid_len ||
  1440		     memcmp(wdev->u.client.ssid, connect->ssid, wdev->u.client.ssid_len)))
  1441			return -EALREADY;
  1442	
  1443		/*
  1444		 * If connected, reject (re-)association unless prev_bssid
  1445		 * matches the current BSSID. Also reject if the current BSSID matches
  1446		 * the desired BSSID.
  1447		 */
  1448		if (wdev->connected) {
  1449			if (!prev_bssid)
  1450				return -EALREADY;
  1451			if (!ether_addr_equal(prev_bssid,
  1452					      wdev->u.client.connected_addr))
  1453				return -ENOTCONN;
> 1454			if (ether_addr_equal(wdev->current_bss->pub.bssid,
  1455					     connect->bssid))
  1456				return -EALREADY;
  1457		}
  1458	
  1459		/*
  1460		 * Reject if we're in the process of connecting with WEP,
  1461		 * this case isn't very interesting and trying to handle
  1462		 * it would make the code much more complex.
  1463		 */
  1464		if (wdev->connect_keys)
  1465			return -EINPROGRESS;
  1466	
  1467		cfg80211_oper_and_ht_capa(&connect->ht_capa_mask,
  1468					  rdev->wiphy.ht_capa_mod_mask);
  1469		cfg80211_oper_and_vht_capa(&connect->vht_capa_mask,
  1470					   rdev->wiphy.vht_capa_mod_mask);
  1471	
  1472		if (connkeys && connkeys->def >= 0) {
  1473			int idx;
  1474			u32 cipher;
  1475	
  1476			idx = connkeys->def;
  1477			cipher = connkeys->params[idx].cipher;
  1478			/* If given a WEP key we may need it for shared key auth */
  1479			if (cipher == WLAN_CIPHER_SUITE_WEP40 ||
  1480			    cipher == WLAN_CIPHER_SUITE_WEP104) {
  1481				connect->key_idx = idx;
  1482				connect->key = connkeys->params[idx].key;
  1483				connect->key_len = connkeys->params[idx].key_len;
  1484	
  1485				/*
  1486				 * If ciphers are not set (e.g. when going through
  1487				 * iwconfig), we have to set them appropriately here.
  1488				 */
  1489				if (connect->crypto.cipher_group == 0)
  1490					connect->crypto.cipher_group = cipher;
  1491	
  1492				if (connect->crypto.n_ciphers_pairwise == 0) {
  1493					connect->crypto.n_ciphers_pairwise = 1;
  1494					connect->crypto.ciphers_pairwise[0] = cipher;
  1495				}
  1496			}
  1497		} else {
  1498			if (WARN_ON(connkeys))
  1499				return -EINVAL;
  1500	
  1501			/* connect can point to wdev->wext.connect which
  1502			 * can hold key data from a previous connection
  1503			 */
  1504			connect->key = NULL;
  1505			connect->key_len = 0;
  1506			connect->key_idx = 0;
  1507		}
  1508	
  1509		wdev->connect_keys = connkeys;
  1510		memcpy(wdev->u.client.ssid, connect->ssid, connect->ssid_len);
  1511		wdev->u.client.ssid_len = connect->ssid_len;
  1512	
  1513		wdev->conn_bss_type = connect->pbss ? IEEE80211_BSS_TYPE_PBSS :
  1514						      IEEE80211_BSS_TYPE_ESS;
  1515	
  1516		if (!rdev->ops->connect)
  1517			err = cfg80211_sme_connect(wdev, connect, prev_bssid);
  1518		else
  1519			err = rdev_connect(rdev, dev, connect);
  1520	
  1521		if (err) {
  1522			wdev->connect_keys = NULL;
  1523			/*
  1524			 * This could be reassoc getting refused, don't clear
  1525			 * ssid_len in that case.
  1526			 */
  1527			if (!wdev->connected)
  1528				wdev->u.client.ssid_len = 0;
  1529			return err;
  1530		}
  1531	
  1532		return 0;
  1533	}
  1534
Johannes Berg June 5, 2023, 4:42 p.m. UTC | #3 
On Fri, 2023-06-02 at 16:57 -0600, Kevin Lund wrote:
> Within cfg80211_connect, reject the (re-)association request if we are
> already connected to the exact BSSID which is being requested. This
> prevents an unnecessary attempt to connect which in the best case
> leaves us back where we started.
> 
> There is precedent for behaving this way over on the userspace SME side
> of things in cfg80211_mlme_auth. Further, cfg80211_connect already makes
> several basic checks to ensure the connection attempt is reasonable, so
> this fits in that context.
> 

I don't think this is right - we should be able to reassoc back to the
same AP in some cases, for example in 11be this comes up when you want
to change the negotiated links.

johannes
Kevin Lund Aug. 4, 2023, 5:29 p.m. UTC | #4 
Thanks for the response. I'm still a little bit confused because
AFAICT, over on the userspace SME side of things there are two
similar checks that reject both authentication [1] and association
[2] to a BSSID which we are already connected to. Wouldn't that
break the 11be flow which you're referring to?

In any case I'm ok with abandoning this patch. I'll resubmit the
other patch in this series [3] on its own.

[1] https://github.com/torvalds/linux/blob/master/net/wireless/mlme.c#L281
[2] https://github.com/torvalds/linux/blob/master/net/wireless/mlme.c#L342
[3] https://patchwork.kernel.org/project/linux-wireless/patch/20230602225751.164525-2-kglund@google.com/

Thanks, and apologies for my late reply,
Kevin

On Mon, Jun 5, 2023 at 10:42 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2023-06-02 at 16:57 -0600, Kevin Lund wrote:
> > Within cfg80211_connect, reject the (re-)association request if we are
> > already connected to the exact BSSID which is being requested. This
> > prevents an unnecessary attempt to connect which in the best case
> > leaves us back where we started.
> >
> > There is precedent for behaving this way over on the userspace SME side
> > of things in cfg80211_mlme_auth. Further, cfg80211_connect already makes
> > several basic checks to ensure the connection attempt is reasonable, so
> > this fits in that context.
> >
>
> I don't think this is right - we should be able to reassoc back to the
> same AP in some cases, for example in 11be this comes up when you want
> to change the negotiated links.
>
> johannes
diff mbox series

Patch

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 7bdeb8eea92dc..8f88e66bc85fc 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -1442,7 +1442,8 @@  int cfg80211_connect(struct cfg80211_registered_device *rdev,
 
 	/*
 	 * If connected, reject (re-)association unless prev_bssid
-	 * matches the current BSSID.
+	 * matches the current BSSID. Also reject if the current BSSID matches
+	 * the desired BSSID.
 	 */
 	if (wdev->connected) {
 		if (!prev_bssid)
@@ -1450,6 +1451,9 @@  int cfg80211_connect(struct cfg80211_registered_device *rdev,
 		if (!ether_addr_equal(prev_bssid,
 				      wdev->u.client.connected_addr))
 			return -ENOTCONN;
+		if (ether_addr_equal(wdev->current_bss->pub.bssid,
+				     connect->bssid))
+			return -EALREADY;
 	}
 
 	/*