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

[John Crispin <john@xxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/hostap

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