[V2,1/2] HE: fix ieee80211_he_capabilities size
diff mbox series

Message ID 20190701132709.18811-1-john@phrozen.org
State Changes Requested
Headers show
Series
  • [V2,1/2] HE: fix ieee80211_he_capabilities size
Related show

Commit Message

John Crispin July 1, 2019, 1:27 p.m. UTC
Set the max value of optional bytes inside the data structure. This
requires us to calculate the actually used size when copying the
HE capabilities and generating the IE.

Signed-off-by: John Crispin <john@phrozen.org>
---
Changes in V2
* drop memset() call

 src/ap/ieee802_11_he.c       | 35 +++++++++++++++++++++++++++++++----
 src/common/ieee802_11_defs.h |  2 +-
 2 files changed, 32 insertions(+), 5 deletions(-)

Comments

Sven Eckelmann July 1, 2019, 5:01 p.m. UTC | #1
On Monday, 1 July 2019 15:27:08 CEST John Crispin wrote:
> @@ -325,7 +353,7 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
>  {
>         if (!he_capab || !hapd->iconf->ieee80211ax ||
>             !check_valid_he_mcs(hapd, he_capab, opmode) ||
> -           he_capab_len > sizeof(struct ieee80211_he_capabilities)) {
> +           ieee80211_check_he_cap_size(he_capab, he_capab_len)) {
>                 sta->flags &= ~WLAN_STA_HE;
>                 os_free(sta->he_capab);
>                 sta->he_capab = NULL;
> @@ -334,13 +362,12 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
>  
>         if (!sta->he_capab) {
>                 sta->he_capab =
> -                       os_zalloc(sizeof(struct ieee80211_he_capabilities));
> +                       os_zalloc(he_capab_len);
>                 if (!sta->he_capab)
>                         return WLAN_STATUS_UNSPECIFIED_FAILURE;
>         }
>  
>         sta->flags |= WLAN_STA_HE;
> -       os_memset(sta->he_capab, 0, sizeof(struct ieee80211_he_capabilities));
>         os_memcpy(sta->he_capab, he_capab, he_capab_len);

See https://patchwork.ozlabs.org/patch/1125264/#2207119

Kind regards,
	Sven
Sven Eckelmann July 2, 2019, 6:44 a.m. UTC | #2
On Monday, 1 July 2019 19:01:56 CEST Sven Eckelmann wrote:
> On Monday, 1 July 2019 15:27:08 CEST John Crispin wrote:
> > @@ -325,7 +353,7 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
> >  {
> >         if (!he_capab || !hapd->iconf->ieee80211ax ||
> >             !check_valid_he_mcs(hapd, he_capab, opmode) ||
> > -           he_capab_len > sizeof(struct ieee80211_he_capabilities)) {
> > +           ieee80211_check_he_cap_size(he_capab, he_capab_len)) {
> >                 sta->flags &= ~WLAN_STA_HE;
> >                 os_free(sta->he_capab);
> >                 sta->he_capab = NULL;
> > @@ -334,13 +362,12 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
> >  
> >         if (!sta->he_capab) {
> >                 sta->he_capab =
> > -                       os_zalloc(sizeof(struct ieee80211_he_capabilities));
> > +                       os_zalloc(he_capab_len);
> >                 if (!sta->he_capab)
> >                         return WLAN_STATUS_UNSPECIFIED_FAILURE;
> >         }
> >  
> >         sta->flags |= WLAN_STA_HE;
> > -       os_memset(sta->he_capab, 0, sizeof(struct ieee80211_he_capabilities));
> >         os_memcpy(sta->he_capab, he_capab, he_capab_len);
> 
> See https://patchwork.ozlabs.org/patch/1125264/#2207119

Got contacted privately by John and told that I should stop these messages and 
either contact him using IRC or send a patch. So here is a patch (not send 
with git-send-email and thus cannot be applied directly):

diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c
index 29a0cfd89..8bbf82692 100644
--- a/src/ap/ieee802_11_he.c
+++ b/src/ap/ieee802_11_he.c
@@ -353,6 +353,7 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
 {
 	if (!he_capab || !hapd->iconf->ieee80211ax ||
 	    !check_valid_he_mcs(hapd, he_capab, opmode) ||
+	    he_capab_len > sizeof(struct ieee80211_he_capabilities) ||
 	    ieee80211_check_he_cap_size(he_capab, he_capab_len)) {
 		sta->flags &= ~WLAN_STA_HE;
 		os_free(sta->he_capab);
@@ -362,12 +363,13 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
 
 	if (!sta->he_capab) {
 		sta->he_capab =
-			os_zalloc(he_capab_len);
+			os_zalloc(sizeof(struct ieee80211_he_capabilities));
 		if (!sta->he_capab)
 			return WLAN_STATUS_UNSPECIFIED_FAILURE;
 	}
 
 	sta->flags |= WLAN_STA_HE;
+	os_memset(sta->he_capab, 0, sizeof(struct ieee80211_he_capabilities));
 	os_memcpy(sta->he_capab, he_capab, he_capab_len);
 	sta->he_capab_len = he_capab_len;

The initial check shouldn't be necessary when ieee80211_check_he_cap_size does 
it job correctly. The memset could be optimized or even dropped. But other 
code sections have to be checked whether they might still try to access the 
uninitialized memory region - I doubt that they do or otherwise original patch
from John would have been a problem too.

Other variant could be:

diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c
index 29a0cfd89..e8b4717cf 100644
--- a/src/ap/ieee802_11_he.c
+++ b/src/ap/ieee802_11_he.c
@@ -360,6 +360,11 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
 		return WLAN_STATUS_SUCCESS;
 	}
 
+	if (sta->he_capab && sta->he_capab_len != he_capab_len) {
+		os_free(sta->he_capab);
+		sta->he_capab = NULL;
+	}
+
 	if (!sta->he_capab) {
 		sta->he_capab =
 			os_zalloc(he_capab_len);

There are various other variants to achieve the same. For example, you could 
avoid the os_free + os_malloc when sta->he_capab_len >= he_capab_len. Or store 
the buffer length in an extra member of struct sta_info ...

Kind regards,
	Sven
John Crispin July 2, 2019, 6:58 a.m. UTC | #3
On 02/07/2019 08:44, Sven Eckelmann wrote:
> On Monday, 1 July 2019 19:01:56 CEST Sven Eckelmann wrote:
>> On Monday, 1 July 2019 15:27:08 CEST John Crispin wrote:
>>> @@ -325,7 +353,7 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
>>>   {
>>>          if (!he_capab || !hapd->iconf->ieee80211ax ||
>>>              !check_valid_he_mcs(hapd, he_capab, opmode) ||
>>> -           he_capab_len > sizeof(struct ieee80211_he_capabilities)) {
>>> +           ieee80211_check_he_cap_size(he_capab, he_capab_len)) {
>>>                  sta->flags &= ~WLAN_STA_HE;
>>>                  os_free(sta->he_capab);
>>>                  sta->he_capab = NULL;
>>> @@ -334,13 +362,12 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
>>>   
>>>          if (!sta->he_capab) {
>>>                  sta->he_capab =
>>> -                       os_zalloc(sizeof(struct ieee80211_he_capabilities));
>>> +                       os_zalloc(he_capab_len);
>>>                  if (!sta->he_capab)
>>>                          return WLAN_STATUS_UNSPECIFIED_FAILURE;
>>>          }
>>>   
>>>          sta->flags |= WLAN_STA_HE;
>>> -       os_memset(sta->he_capab, 0, sizeof(struct ieee80211_he_capabilities));
>>>          os_memcpy(sta->he_capab, he_capab, he_capab_len);
>> See https://patchwork.ozlabs.org/patch/1125264/#2207119
> Got contacted privately by John and told that I should stop these messages and
> either contact him using IRC or send a patch. So here is a patch (not send
> with git-send-email and thus cannot be applied directly):
>
> diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c
> index 29a0cfd89..8bbf82692 100644
> --- a/src/ap/ieee802_11_he.c
> +++ b/src/ap/ieee802_11_he.c
> @@ -353,6 +353,7 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
>   {
>   	if (!he_capab || !hapd->iconf->ieee80211ax ||
>   	    !check_valid_he_mcs(hapd, he_capab, opmode) ||
> +	    he_capab_len > sizeof(struct ieee80211_he_capabilities) ||
>   	    ieee80211_check_he_cap_size(he_capab, he_capab_len)) {
>   		sta->flags &= ~WLAN_STA_HE;
>   		os_free(sta->he_capab);
> @@ -362,12 +363,13 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
>   
>   	if (!sta->he_capab) {
>   		sta->he_capab =
> -			os_zalloc(he_capab_len);
> +			os_zalloc(sizeof(struct ieee80211_he_capabilities));
>   		if (!sta->he_capab)
>   			return WLAN_STATUS_UNSPECIFIED_FAILURE;
>   	}
>   
>   	sta->flags |= WLAN_STA_HE;
> +	os_memset(sta->he_capab, 0, sizeof(struct ieee80211_he_capabilities));
>   	os_memcpy(sta->he_capab, he_capab, he_capab_len);
>   	sta->he_capab_len = he_capab_len;
>
> The initial check shouldn't be necessary when ieee80211_check_he_cap_size does
> it job correctly. The memset could be optimized or even dropped. But other
> code sections have to be checked whether they might still try to access the
> uninitialized memory region - I doubt that they do or otherwise original patch
> from John would have been a problem too.
>
> Other variant could be:
>
> diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c
> index 29a0cfd89..e8b4717cf 100644
> --- a/src/ap/ieee802_11_he.c
> +++ b/src/ap/ieee802_11_he.c
> @@ -360,6 +360,11 @@ u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
>   		return WLAN_STATUS_SUCCESS;
>   	}
>   
> +	if (sta->he_capab && sta->he_capab_len != he_capab_len) {
> +		os_free(sta->he_capab);
> +		sta->he_capab = NULL;
> +	}
> +
>   	if (!sta->he_capab) {
>   		sta->he_capab =
>   			os_zalloc(he_capab_len);
>
> There are various other variants to achieve the same. For example, you could
> avoid the os_free + os_malloc when sta->he_capab_len >= he_capab_len. Or store
> the buffer length in an extra member of struct sta_info ...
>
> Kind regards,
> 	Sven
>

thanks, let me fold it into a V3 with my patch

     John


> _______________________________________________
> Hostap mailing list
> Hostap@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/hostap
Jouni Malinen Aug. 11, 2019, 2:57 p.m. UTC | #4
On Tue, Jul 02, 2019 at 08:58:53AM +0200, John Crispin wrote:
> thanks, let me fold it into a V3 with my patch

I have not seen V3 of this patch so far.. Does that exist? In any case,
I'm dropping v1 and v2 of "HE: fix ieee80211_he_capabilities size" from
my queue with the expectation that an updated version will be posted at
some point.

Patch
diff mbox series

diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c
index ab1ac72f6..344a23347 100644
--- a/src/ap/ieee802_11_he.c
+++ b/src/ap/ieee802_11_he.c
@@ -43,6 +43,34 @@  static u8 ieee80211_he_ppet_size(u8 ppe_thres_hdr, const u8 *phy_cap_info)
 	return sz;
 }
 
+static u8 ieee80211_he_mcs_set_size(const u8 *phy_cap_info)
+{
+	u8 sz = 4;
+
+	if (phy_cap_info[HE_PHYCAP_CHANNEL_WIDTH_SET_IDX] & HE_PHYCAP_CHANNEL_WIDTH_SET_80PLUS80MHZ_IN_5G)
+		sz += 4;
+	if (phy_cap_info[HE_PHYCAP_CHANNEL_WIDTH_SET_IDX] & HE_PHYCAP_CHANNEL_WIDTH_SET_160MHZ_IN_5G)
+		sz += 4;
+
+	return sz;
+}
+
+static int ieee80211_check_he_cap_size(const u8 *buf, int len)
+{
+	struct ieee80211_he_capabilities *cap = (struct ieee80211_he_capabilities *)buf;
+	int cap_len = sizeof(*cap) - sizeof(cap->optional);
+
+	if (len < cap_len)
+		return 1;
+
+	cap_len += ieee80211_he_mcs_set_size(cap->he_phy_capab_info);
+	if (len < cap_len)
+		return 1;
+
+	cap_len += ieee80211_he_ppet_size(buf[cap_len], cap->he_phy_capab_info);
+
+	return (len != cap_len);
+}
 
 u8 * hostapd_eid_he_capab(struct hostapd_data *hapd, u8 *eid,
 			  enum ieee80211_op_mode opmode)
@@ -56,7 +84,7 @@  u8 * hostapd_eid_he_capab(struct hostapd_data *hapd, u8 *eid,
 	if (!mode)
 		return eid;
 
-	ie_size = sizeof(struct ieee80211_he_capabilities);
+	ie_size = sizeof(*cap) - sizeof(cap->optional);
 	ppet_size = ieee80211_he_ppet_size(mode->he_capab[opmode].ppet[0],
 					   mode->he_capab[opmode].phy_cap);
 
@@ -325,7 +353,7 @@  u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
 {
 	if (!he_capab || !hapd->iconf->ieee80211ax ||
 	    !check_valid_he_mcs(hapd, he_capab, opmode) ||
-	    he_capab_len > sizeof(struct ieee80211_he_capabilities)) {
+	    ieee80211_check_he_cap_size(he_capab, he_capab_len)) {
 		sta->flags &= ~WLAN_STA_HE;
 		os_free(sta->he_capab);
 		sta->he_capab = NULL;
@@ -334,13 +362,12 @@  u16 copy_sta_he_capab(struct hostapd_data *hapd, struct sta_info *sta,
 
 	if (!sta->he_capab) {
 		sta->he_capab =
-			os_zalloc(sizeof(struct ieee80211_he_capabilities));
+			os_zalloc(he_capab_len);
 		if (!sta->he_capab)
 			return WLAN_STATUS_UNSPECIFIED_FAILURE;
 	}
 
 	sta->flags |= WLAN_STA_HE;
-	os_memset(sta->he_capab, 0, sizeof(struct ieee80211_he_capabilities));
 	os_memcpy(sta->he_capab, he_capab, he_capab_len);
 	sta->he_capab_len = he_capab_len;
 
diff --git a/src/common/ieee802_11_defs.h b/src/common/ieee802_11_defs.h
index 12c004f88..1d302559e 100644
--- a/src/common/ieee802_11_defs.h
+++ b/src/common/ieee802_11_defs.h
@@ -2109,7 +2109,7 @@  struct ieee80211_he_capabilities {
 	u8 he_phy_capab_info[11];
 	/* Followed by 4, 8, or 12 octets of Supported HE-MCS And NSS Set field
 	* and optional variable length PPE Thresholds field. */
-	u8 optional[];
+	u8 optional[37];
 } STRUCT_PACKED;
 
 struct ieee80211_he_operation {