[patch] rndis_wlan: add range check in del_key()

[patch] rndis_wlan: add range check in del_key()

[Dan Carpenter]

Wifi drivers can have up to 6 keys but the rndis_wlan only has 4 so
it needs to have its own checks to make sure we don't go out of
bounds.  The add_key() function already checks but I added some
checks to del_key() and set_default_key().

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
index 0c13840..da78369 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -414,6 +414,7 @@ struct ndis_80211_pmkid {
 #define RNDIS_WLAN_ALG_TKIP	(1<<1)
 #define RNDIS_WLAN_ALG_CCMP	(1<<2)
 
+#define RNDIS_WLAN_NUM_KEYS		4
 #define RNDIS_WLAN_KEY_MGMT_NONE	0
 #define RNDIS_WLAN_KEY_MGMT_802_1X	(1<<0)
 #define RNDIS_WLAN_KEY_MGMT_PSK		(1<<1)
@@ -516,7 +517,7 @@ struct rndis_wlan_private {
 
 	/* encryption stuff */
 	int  encr_tx_key_index;
-	struct rndis_wlan_encr_key encr_keys[4];
+	struct rndis_wlan_encr_key encr_keys[RNDIS_WLAN_NUM_KEYS];
 	int  wpa_version;
 
 	u8 command_buffer[COMMAND_BUFFER_SIZE];
@@ -1535,6 +1536,9 @@ static int remove_key(struct usbnet *usbdev, int index, const u8 *bssid)
 	bool is_wpa;
 	int ret;
 
+	if (index >= RNDIS_WLAN_NUM_KEYS)
+		return -ENOENT;
+
 	if (priv->encr_keys[index].len == 0)
 		return 0;
 
@@ -2451,6 +2455,9 @@ static int rndis_set_default_key(struct wiphy *wiphy, struct net_device *netdev,
 
 	netdev_dbg(usbdev->net, "%s(%i)\n", __func__, key_index);
 
+	if (key_index >= RNDIS_WLAN_NUM_KEYS)
+		return -ENOENT;
+
 	priv->encr_tx_key_index = key_index;
 
 	if (is_wpa_key(priv, key_index))

Re: [patch] rndis_wlan: add range check in del_key()

[Johannes Berg]

On Tue, 2011-10-18 at 09:47 +0300, Dan Carpenter wrote:
> Wifi drivers can have up to 6 keys but the rndis_wlan only has 4 so
> it needs to have its own checks to make sure we don't go out of
> bounds.  The add_key() function already checks but I added some
> checks to del_key() and set_default_key().

Semantically, that shouldn't be possible unless it advertises support
for WLAN_CIPHER_SUITE_AES_CMAC. Is there a bug in those checks?

The checks don't hurt, obviously.

johannes

Re: [patch] rndis_wlan: add range check in del_key()

[Jussi Kivilinna]

Quoting Dan Carpenter <dan.carpenter@oracle.com>:

> Wifi drivers can have up to 6 keys but the rndis_wlan only has 4 so
> it needs to have its own checks to make sure we don't go out of
> bounds.  The add_key() function already checks but I added some
> checks to del_key() and set_default_key().

Looks ok, thanks.

Acked-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>

>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/net/wireless/rndis_wlan.c  
> b/drivers/net/wireless/rndis_wlan.c
> index 0c13840..da78369 100644
> --- a/drivers/net/wireless/rndis_wlan.c
> +++ b/drivers/net/wireless/rndis_wlan.c
> @@ -414,6 +414,7 @@ struct ndis_80211_pmkid {
>  #define RNDIS_WLAN_ALG_TKIP	(1<<1)
>  #define RNDIS_WLAN_ALG_CCMP	(1<<2)
>
> +#define RNDIS_WLAN_NUM_KEYS		4
>  #define RNDIS_WLAN_KEY_MGMT_NONE	0
>  #define RNDIS_WLAN_KEY_MGMT_802_1X	(1<<0)
>  #define RNDIS_WLAN_KEY_MGMT_PSK		(1<<1)
> @@ -516,7 +517,7 @@ struct rndis_wlan_private {
>
>  	/* encryption stuff */
>  	int  encr_tx_key_index;
> -	struct rndis_wlan_encr_key encr_keys[4];
> +	struct rndis_wlan_encr_key encr_keys[RNDIS_WLAN_NUM_KEYS];
>  	int  wpa_version;
>
>  	u8 command_buffer[COMMAND_BUFFER_SIZE];
> @@ -1535,6 +1536,9 @@ static int remove_key(struct usbnet *usbdev,  
> int index, const u8 *bssid)
>  	bool is_wpa;
>  	int ret;
>
> +	if (index >= RNDIS_WLAN_NUM_KEYS)
> +		return -ENOENT;
> +
>  	if (priv->encr_keys[index].len == 0)
>  		return 0;
>
> @@ -2451,6 +2455,9 @@ static int rndis_set_default_key(struct wiphy  
> *wiphy, struct net_device *netdev,
>
>  	netdev_dbg(usbdev->net, "%s(%i)\n", __func__, key_index);
>
> +	if (key_index >= RNDIS_WLAN_NUM_KEYS)
> +		return -ENOENT;
> +
>  	priv->encr_tx_key_index = key_index;
>
>  	if (is_wpa_key(priv, key_index))
>
>

Re: [patch] rndis_wlan: add range check in del_key()

[Dan Carpenter]

On Tue, Oct 18, 2011 at 08:54:47AM +0200, Johannes Berg wrote:
> On Tue, 2011-10-18 at 09:47 +0300, Dan Carpenter wrote:
> > Wifi drivers can have up to 6 keys but the rndis_wlan only has 4 so
> > it needs to have its own checks to make sure we don't go out of
> > bounds.  The add_key() function already checks but I added some
> > checks to del_key() and set_default_key().
> 
> Semantically, that shouldn't be possible unless it advertises support
> for WLAN_CIPHER_SUITE_AES_CMAC. Is there a bug in those checks?
> 

You know I'm a newbie at this networking...  I obviously had no idea
about WLAN_CIPHER_SUITE_AES_CMAC until you mentioned it.  I just
looked at the other implementations of del_key() etc and they checked
it.  Monkey see, monkey do.

My concern when I wrote this patch was places like __cfg80211_clear_ibss()
which just do:

	for (i = 0; i < 6; i++)
		rdev->ops->del_key(wdev->wiphy, dev, i, false, NULL);

That's what triggers the Smatch warning as well.  But as I said, I'm
quite a newbie at this code.

regards,
dan carpenter

Re: [patch] rndis_wlan: add range check in del_key()

[Johannes Berg]

On Tue, 2011-10-18 at 10:22 +0300, Dan Carpenter wrote:
> On Tue, Oct 18, 2011 at 08:54:47AM +0200, Johannes Berg wrote:
> > On Tue, 2011-10-18 at 09:47 +0300, Dan Carpenter wrote:
> > > Wifi drivers can have up to 6 keys but the rndis_wlan only has 4 so
> > > it needs to have its own checks to make sure we don't go out of
> > > bounds.  The add_key() function already checks but I added some
> > > checks to del_key() and set_default_key().
> > 
> > Semantically, that shouldn't be possible unless it advertises support
> > for WLAN_CIPHER_SUITE_AES_CMAC. Is there a bug in those checks?
> > 
> 
> You know I'm a newbie at this networking...  I obviously had no idea
> about WLAN_CIPHER_SUITE_AES_CMAC until you mentioned it.  I just
> looked at the other implementations of del_key() etc and they checked
> it.  Monkey see, monkey do.
> 
> My concern when I wrote this patch was places like __cfg80211_clear_ibss()
> which just do:
> 
> 	for (i = 0; i < 6; i++)
> 		rdev->ops->del_key(wdev->wiphy, dev, i, false, NULL);
> 
> That's what triggers the Smatch warning as well.  But as I said, I'm
> quite a newbie at this code.

Ah ok, I didn't know they didn't check. That's reasonable, thanks!

I was just thinking that we wouldn't have keys 4/5 w/o CMAC to start
with, but it makes some sense that the code would just try to clear all
keys.

johannes