* question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie
@ 2015-09-29 15:21 ` PaX Team
0 siblings, 0 replies; 7+ messages in thread
From: PaX Team @ 2015-09-29 15:21 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: Avinash Patil, Kalle Valo, linux-wireless, netdev, re.emese, spender
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.
FTR, this issue was detected with the upcoming version of the size overflow
plugin we have in PaX/grsecurity and there're a handful of similar cases in
the tree where potentially unwanted or unnecessary integer truncations occur,
this being one of these. any opinion/help is welcome!
cheers,
PaX Team
^ permalink raw reply [flat|nested] 7+ messages in thread
* question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie
@ 2015-09-29 15:21 ` PaX Team
0 siblings, 0 replies; 7+ messages in thread
From: PaX Team @ 2015-09-29 15:21 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: Avinash Patil, Kalle Valo, linux-wireless, netdev, re.emese, spender
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.
FTR, this issue was detected with the upcoming version of the size overflow
plugin we have in PaX/grsecurity and there're a handful of similar cases in
the tree where potentially unwanted or unnecessary integer truncations occur,
this being one of these. any opinion/help is welcome!
cheers,
PaX Team
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie
@ 2015-09-29 23:10 ` James Cameron
0 siblings, 0 replies; 7+ messages in thread
From: James Cameron @ 2015-09-29 23:10 UTC (permalink / raw)
To: PaX Team
Cc: Amitkumar Karwar, Avinash Patil, Kalle Valo, linux-wireless,
netdev, re.emese, spender
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;
--
James Cameron
http://quozl.linux.org.au/
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie
@ 2015-09-29 23:10 ` James Cameron
0 siblings, 0 replies; 7+ messages in thread
From: James Cameron @ 2015-09-29 23:10 UTC (permalink / raw)
To: PaX Team
Cc: Amitkumar Karwar, Avinash Patil, Kalle Valo,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, re.emese-Re5JQEeQqe8AvxtiuMwx3w,
spender-JNS0hek0TMl4qEwOxq4T+Q
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;
--
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie
2015-09-29 23:10 ` James Cameron
@ 2015-09-30 15:13 ` PaX Team
-1 siblings, 0 replies; 7+ messages in thread
From: PaX Team @ 2015-09-30 15:13 UTC (permalink / raw)
To: James Cameron
Cc: Amitkumar Karwar, Avinash Patil, Kalle Valo, linux-wireless,
netdev, re.emese, spender
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/
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie
@ 2015-09-30 15:13 ` PaX Team
0 siblings, 0 replies; 7+ messages in thread
From: PaX Team @ 2015-09-30 15:13 UTC (permalink / raw)
To: James Cameron
Cc: Amitkumar Karwar, Avinash Patil, Kalle Valo, linux-wireless,
netdev, re.emese, spender
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/
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie
2015-09-29 23:10 ` James Cameron
(?)
(?)
@ 2015-10-09 12:05 ` Amitkumar Karwar
-1 siblings, 0 replies; 7+ messages in thread
From: Amitkumar Karwar @ 2015-10-09 12:05 UTC (permalink / raw)
To: quozl, PaX Team
Cc: Avinash Patil, Kalle Valo, linux-wireless, netdev, re.emese, spender
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-10-09 12:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-29 15:21 question about potential integer truncation in mwifiex_set_wapi_ie and mwifiex_set_wps_ie PaX Team
2015-09-29 15:21 ` PaX Team
2015-09-29 23:10 ` James Cameron
2015-09-29 23:10 ` James Cameron
2015-09-30 15:13 ` PaX Team
2015-09-30 15:13 ` PaX Team
2015-10-09 12:05 ` Amitkumar Karwar
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.