| Message ID | 20190701132709.18811-1-john@phrozen.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V2,1/2] HE: fix ieee80211_he_capabilities size | expand |
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
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
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
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.
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 {
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(-)