Re: [PATCH V2 1/2] HE: fix ieee80211_he_capabilities size

[Sven Eckelmann <sven@xxxxxxxxxxxxx>]

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

Attachment: signature.asc
Description: This is a digitally signed message part.

_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap