| Message ID | 20150929231038.GG8613@us.netrek.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On 30 Sep 2015 at 9:10, James Cameron wrote: > On Tue, Sep 29, 2015 at 05:21:28PM +0200, PaX Team wrote: > > hi all, > > > > in drivers/net/wireless/mwifiex/sta_ioctl.c the following functions > > > > mwifiex_set_wpa_ie_helper > > mwifiex_set_wapi_ie > > mwifiex_set_wps_ie > > > > can truncate the incoming ie_len argument from u16 to u8 when it gets > > stored in mwifiex_private.wpa_ie_len, mwifiex_private.wapi_ie_len and > > mwifiex_private.wps_ie_len, respectively. based on some light code > > reading it seems a length value of 256 is valid (IEEE_MAX_IE_SIZE and > > MWIFIEX_MAX_VSIE_LEN seem to limit it) and thus would get truncated > > to 0 when stored in those u8 fields. the question is whether this is > > intentional or a bug somewhere. > > i agree, while there is a test to ensure ie_len is not greater than > 256, there is a possibility that it will be exactly 256, which means > 256 bytes will be given to memcpy but > mwifiex_private.{wpa,wapi,wps}_ie_len will be zero. > > i suggest changing the lengths to u16. not tested. while this is probably a necessary first step you'll also have to verify all loads from these fields for integer truncation that can occur now (the stores were checked by the plugin i mentioned above). based on a naive grep the code seems to be fine but someone with actual knowledge of the code had better verify it ;). > diff --git a/drivers/net/wireless/mwifiex/main.h b/drivers/net/wireless/mwifiex/main.h > index fe12560..b66e9a7 100644 > --- a/drivers/net/wireless/mwifiex/main.h > +++ b/drivers/net/wireless/mwifiex/main.h > @@ -512,14 +512,14 @@ struct mwifiex_private { > struct mwifiex_wep_key wep_key[NUM_WEP_KEYS]; > u16 wep_key_curr_index; > u8 wpa_ie[256]; > - u8 wpa_ie_len; > + u16 wpa_ie_len; > u8 wpa_is_gtk_set; > struct host_cmd_ds_802_11_key_material aes_key; > struct host_cmd_ds_802_11_key_material_v2 aes_key_v2; > u8 wapi_ie[256]; > - u8 wapi_ie_len; > + u16 wapi_ie_len; > u8 *wps_ie; > - u8 wps_ie_len; > + u16 wps_ie_len; > u8 wmm_required; > u8 wmm_enabled; > u8 wmm_qosinfo; > > -- > James Cameron > http://quozl.linux.org.au/ > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi James/PaX Team, > -----Original Message----- > From: quozl@laptop.org [mailto:quozl@laptop.org] > Sent: Wednesday, September 30, 2015 4:41 AM > To: PaX Team > Cc: Amitkumar Karwar; Avinash Patil; Kalle Valo; linux- > wireless@vger.kernel.org; netdev@vger.kernel.org; re.emese@gmail.com; > spender@grsecurity.net > Subject: Re: question about potential integer truncation in > mwifiex_set_wapi_ie and mwifiex_set_wps_ie > > On Tue, Sep 29, 2015 at 05:21:28PM +0200, PaX Team wrote: > > hi all, > > > > in drivers/net/wireless/mwifiex/sta_ioctl.c the following functions > > > > mwifiex_set_wpa_ie_helper > > mwifiex_set_wapi_ie > > mwifiex_set_wps_ie > > > > can truncate the incoming ie_len argument from u16 to u8 when it gets > > stored in mwifiex_private.wpa_ie_len, mwifiex_private.wapi_ie_len and > > mwifiex_private.wps_ie_len, respectively. based on some light code > > reading it seems a length value of 256 is valid (IEEE_MAX_IE_SIZE and > > MWIFIEX_MAX_VSIE_LEN seem to limit it) and thus would get truncated to > > 0 when stored in those u8 fields. the question is whether this is > > intentional or a bug somewhere. > > i agree, while there is a test to ensure ie_len is not greater than 256, > there is a possibility that it will be exactly 256, which means > 256 bytes will be given to memcpy but > mwifiex_private.{wpa,wapi,wps}_ie_len will be zero. > > i suggest changing the lengths to u16. not tested. > > diff --git a/drivers/net/wireless/mwifiex/main.h > b/drivers/net/wireless/mwifiex/main.h > index fe12560..b66e9a7 100644 > --- a/drivers/net/wireless/mwifiex/main.h > +++ b/drivers/net/wireless/mwifiex/main.h > @@ -512,14 +512,14 @@ struct mwifiex_private { > struct mwifiex_wep_key wep_key[NUM_WEP_KEYS]; > u16 wep_key_curr_index; > u8 wpa_ie[256]; > - u8 wpa_ie_len; > + u16 wpa_ie_len; > u8 wpa_is_gtk_set; > struct host_cmd_ds_802_11_key_material aes_key; > struct host_cmd_ds_802_11_key_material_v2 aes_key_v2; > u8 wapi_ie[256]; > - u8 wapi_ie_len; > + u16 wapi_ie_len; > u8 *wps_ie; > - u8 wps_ie_len; > + u16 wps_ie_len; > u8 wmm_required; > u8 wmm_enabled; > u8 wmm_qosinfo; > This change makes sense. Also, we should not typecast the length to v8 while copying to mwifiex_private variable. ./sta_ioctl.c:761: priv->wpa_ie_len = (u8) ie_len; Eventually the length stored in 'wapi_ie_len' is copied to a u16 variable. /join.c:304: ie_header.len = cpu_to_le16(priv->wapi_ie_len); I will submit a patch to fix this. Regards, Amitkumar -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/mwifiex/main.h b/drivers/net/wireless/mwifiex/main.h index fe12560..b66e9a7 100644 --- a/drivers/net/wireless/mwifiex/main.h +++ b/drivers/net/wireless/mwifiex/main.h @@ -512,14 +512,14 @@ struct mwifiex_private { struct mwifiex_wep_key wep_key[NUM_WEP_KEYS]; u16 wep_key_curr_index; u8 wpa_ie[256]; - u8 wpa_ie_len; + u16 wpa_ie_len; u8 wpa_is_gtk_set; struct host_cmd_ds_802_11_key_material aes_key; struct host_cmd_ds_802_11_key_material_v2 aes_key_v2; u8 wapi_ie[256]; - u8 wapi_ie_len; + u16 wapi_ie_len; u8 *wps_ie; - u8 wps_ie_len; + u16 wps_ie_len; u8 wmm_required; u8 wmm_enabled; u8 wmm_qosinfo;