| Message ID | 20190701123926.25549-1-john@phrozen.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] HE: fix ieee80211_he_capabilities size | expand |
Hi John, I guess the "os_memset(sta->he_capab, 0, he_capab_len);" is not really necessary now (even if sta->he_capab was already allocated before and is currently dirty) as the next code line is: "os_memcpy(sta->he_capab, he_capab, he_capab_len);" Thanks, Shay Bar SW Engineer Celeno Communications (tm) shay.bar@celeno.com The information transmitted is intended only for the person or entity to which it is addressed and may contain confidential and/or privileged material. Any retransmission, dissemination, copying or other use of, or taking of any action in reliance upon this information is prohibited. If you received this in error, please contact the sender and delete the material from any computer. Nothing contained herein shall be deemed as a representation, warranty or a commitment by Celeno. No warranties are expressed or implied, including, but not limited to, any implied warranties of non-infringement, merchantability and fitness for a particular purpose. -----Original Message----- From: Hostap <hostap-bounces@lists.infradead.org> On Behalf Of John Crispin Sent: Monday, 1 July 2019 15:39 To: Jouni Malinen <j@w1.fi> Cc: hostap@lists.infradead.org; John Crispin <john@phrozen.org> Subject: [PATCH 1/2] HE: fix ieee80211_he_capabilities size External Email 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> --- src/ap/ieee802_11_he.c | 36 ++++++++++++++++++++++++++++++++---- src/common/ieee802_11_defs.h | 2 +- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c index ab1ac72f6..bbf5d14b1 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,13 @@ 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_memset(sta->he_capab, 0, he_capab_len); 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 { -- 2.20.1
On Monday, 1 July 2019 14:39:25 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,13 @@ 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_memset(sta->he_capab, 0, he_capab_len); > os_memcpy(sta->he_capab, he_capab, he_capab_len); > sta->he_capab_len = he_capab_len; Isn't this creating the same sta->he_capab size uncertainty which Jouni previously found in the old version of the patch [1]. Kind regards, Sven [1] https://patchwork.ozlabs.org/patch/1109462/
On 01/07/2019 15:00, Sven Eckelmann wrote: > On Monday, 1 July 2019 14:39:25 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,13 @@ 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_memset(sta->he_capab, 0, he_capab_len); >> os_memcpy(sta->he_capab, he_capab, he_capab_len); >> sta->he_capab_len = he_capab_len; > Isn't this creating the same sta->he_capab size uncertainty which Jouni > previously found in the old version of the patch [1]. > > Kind regards, > Sven > no, the call to ieee80211_check_he_cap_size(he_capab, he_capab_len) will make sure the length is valid John > [1] https://patchwork.ozlabs.org/patch/1109462/ > > _______________________________________________ > Hostap mailing list > Hostap@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/hostap
On Monday, 1 July 2019 15:13:48 CEST John Crispin wrote: > > On 01/07/2019 15:00, Sven Eckelmann wrote: > > On Monday, 1 July 2019 14:39:25 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,13 @@ 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_memset(sta->he_capab, 0, he_capab_len); > >> os_memcpy(sta->he_capab, he_capab, he_capab_len); > >> sta->he_capab_len = he_capab_len; > > Isn't this creating the same sta->he_capab size uncertainty which Jouni > > previously found in the old version of the patch [1]. > > > > Kind regards, > > Sven > > > no, the call to ieee80211_check_he_cap_size(he_capab, he_capab_len) will > make sure the length is valid I think we are talking about two different things. sta->he_capab is either NULL or has a size X. This size X is not checked anywhere. Please think about following scenario: * first round of copy_sta_he_capab for sta - copy_sta_he_capab is called for size X, sta->he_capab is NULL - ieee80211_check_he_cap_size doesn't detect wrong he_capab_len (he_capab requires size X == he_capab_len) - sta->he_capab is allocated with size X - memcpy happens with size X * second round of copy_sta_he_capab for sta - copy_sta_he_capab is called for size Y (Y > X), sta->he_capab is of size Y - ieee80211_check_he_cap_size doesn't detect wrong he_capab_len (he_capab requires size Y == he_capab_len) - sta->he_capab is not reallocated - memcpy happens with size Y -> *buffer overflow on heap* Kind regards, Sven
On 01/07/2019 18:56, Sven Eckelmann wrote: > On Monday, 1 July 2019 15:13:48 CEST John Crispin wrote: >> On 01/07/2019 15:00, Sven Eckelmann wrote: >>> On Monday, 1 July 2019 14:39:25 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,13 @@ 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_memset(sta->he_capab, 0, he_capab_len); >>>> os_memcpy(sta->he_capab, he_capab, he_capab_len); >>>> sta->he_capab_len = he_capab_len; >>> Isn't this creating the same sta->he_capab size uncertainty which Jouni >>> previously found in the old version of the patch [1]. >>> >>> Kind regards, >>> Sven >>> >> no, the call to ieee80211_check_he_cap_size(he_capab, he_capab_len) will >> make sure the length is valid > I think we are talking about two different things. sta->he_capab is either > NULL or has a size X. This size X is not checked anywhere. Please think about > following scenario: > > * first round of copy_sta_he_capab for sta > - copy_sta_he_capab is called for size X, sta->he_capab is NULL > - ieee80211_check_he_cap_size doesn't detect wrong he_capab_len > (he_capab requires size X == he_capab_len) > - sta->he_capab is allocated with size X > - memcpy happens with size X > * second round of copy_sta_he_capab for sta > - copy_sta_he_capab is called for size Y (Y > X), sta->he_capab is of size Y > - ieee80211_check_he_cap_size doesn't detect wrong he_capab_len > (he_capab requires size Y == he_capab_len) > - sta->he_capab is not reallocated > - memcpy happens with size Y -> *buffer overflow on heap* > > Kind regards, > Sven > I'll look into this in the morning and send a V3 John
diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c index ab1ac72f6..bbf5d14b1 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,13 @@ 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_memset(sta->he_capab, 0, he_capab_len); 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> --- src/ap/ieee802_11_he.c | 36 ++++++++++++++++++++++++++++++++---- src/common/ieee802_11_defs.h | 2 +- 2 files changed, 33 insertions(+), 5 deletions(-)