| Message ID | 20190603192117.26416-1-john@phrozen.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | HE: fix he_capabilities size | expand |
On Monday, 3 June 2019 21:21:17 CEST John Crispin wrote: > The ppet field inside ieee80211_he_capabilities is of size [0]. The code > currently copies up to 12 additional bytes into the buffer, thus overwriting > memory. Fix this by verifying the size properly and using the passed length > value for allocation and the following memcpy() call. > > Signed-off-by: John Crispin <john@phrozen.org> > --- > src/ap/ieee802_11_he.c | 39 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) Ran into the same problem and tested it together with https://patchwork.ozlabs.org/patch/1114908/ to get the HE mesh new peer capability handling working. Tested-by: Sven Eckelmann <seckelmann@datto.com> Kind regards, Sven
On Mon, Jun 03, 2019 at 09:21:17PM +0200, John Crispin wrote: > The ppet field inside ieee80211_he_capabilities is of size [0]. The code > currently copies up to 12 additional bytes into the buffer, thus overwriting > memory. Fix this by verifying the size properly and using the passed length > value for allocation and the following memcpy() call. What is the relationship of this patch to "[V2] HE: fix hostapd_get_he_capab()"? http://patchwork.ozlabs.org/patch/1116968/ That patch proposes changes to the optional[] array in the end of struct ieee80211_he_capabilities to make it fixed length 37.. > +static inline u8 > +ieee80211_he_mcs_set_size(const u8 *phy_cap_info) Please no "inline" in helper functions. The compiler should be clever enough to handle this for static functions. > @@ -331,13 +364,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); What guarantees that the allocated sta->he_capab buffer is large enough if it can now be of variable size and it is not reallocated in case sta->he_capab was non-NULL when getting here? That part would work with fixed size allocations, but this one does not seem safe without this function modified to check that there is sufficient room or alternatively, always allocate the maximum size or always allocate the buffer again here regardless of whether it was previously allocated.
On Monday, 3 June 2019 21:21:17 CEST John Crispin wrote: > The ppet field inside ieee80211_he_capabilities is of size [0]. The code > currently copies up to 12 additional bytes into the buffer, thus overwriting > memory. Fix this by verifying the size properly and using the passed length > value for allocation and the following memcpy() call. > > Signed-off-by: John Crispin <john@phrozen.org> I just went to the patches which might be interesting for me to get HE in a working state for ath11k mesh. And so I found following patches which seemed to be relevant for me: * https://patchwork.ozlabs.org/patch/1109462/ (this patched; marked as "changes requested") * https://patchwork.ozlabs.org/patch/1122057/ (marked superseded) It looks to me that the mentioned patches can all be replaced by https://patchwork.ozlabs.org/patch/1116968/ - is this assumption correct or am I missing something? [...] > diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c > index ba22a174a..63270228f 100644 > --- a/src/ap/ieee802_11_he.c > +++ b/src/ap/ieee802_11_he.c > @@ -44,6 +44,39 @@ static u8 ieee80211_he_ppet_size(u8 ppe_thres_hdr, const u8 *phy_cap_info) > } > > > +static inline u8 > +ieee80211_he_mcs_set_size(const u8 *phy_cap_info) > +{ We talked about this on a call (were I had problems to understand you) and I was under the impression that you were talking about static inline functions in headers. So I have to retract my statement and agree with Jouni about this. Kind regards, Sven
I agree that https://patchwork.ozlabs.org/patch/1116968/ can replace both: https://patchwork.ozlabs.org/patch/1109462/ https://patchwork.ozlabs.org/patch/1122057/ 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: Sven Eckelmann <sven@narfation.org> Sent: Thursday, 27 June 2019 11:12 To: hostap@lists.infradead.org Cc: John Crispin <john@phrozen.org>; Shay Bar <Shay.Bar@celeno.com> Subject: Re: [PATCH] HE: fix he_capabilities size On Monday, 3 June 2019 21:21:17 CEST John Crispin wrote: > The ppet field inside ieee80211_he_capabilities is of size [0]. The > code currently copies up to 12 additional bytes into the buffer, thus > overwriting memory. Fix this by verifying the size properly and using > the passed length value for allocation and the following memcpy() call. > > Signed-off-by: John Crispin <john@phrozen.org> I just went to the patches which might be interesting for me to get HE in a working state for ath11k mesh. And so I found following patches which seemed to be relevant for me: * https://patchwork.ozlabs.org/patch/1109462/ (this patched; marked as "changes requested") * https://patchwork.ozlabs.org/patch/1122057/ (marked superseded) It looks to me that the mentioned patches can all be replaced by https://patchwork.ozlabs.org/patch/1116968/ - is this assumption correct or am I missing something? [...] > diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c index > ba22a174a..63270228f 100644 > --- a/src/ap/ieee802_11_he.c > +++ b/src/ap/ieee802_11_he.c > @@ -44,6 +44,39 @@ static u8 ieee80211_he_ppet_size(u8 ppe_thres_hdr, > const u8 *phy_cap_info) > } > > > +static inline u8 > +ieee80211_he_mcs_set_size(const u8 *phy_cap_info) { We talked about this on a call (were I had problems to understand you) and I was under the impression that you were talking about static inline functions in headers. So I have to retract my statement and agree with Jouni about this. Kind regards, Sven
On 27/06/2019 10:32, Shay Bar wrote: > I agree that https://patchwork.ozlabs.org/patch/1116968/ can replace both: > https://patchwork.ozlabs.org/patch/1109462/ > https://patchwork.ozlabs.org/patch/1122057/ > > 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: Sven Eckelmann <sven@narfation.org> > Sent: Thursday, 27 June 2019 11:12 > To: hostap@lists.infradead.org > Cc: John Crispin <john@phrozen.org>; Shay Bar <Shay.Bar@celeno.com> > Subject: Re: [PATCH] HE: fix he_capabilities size > > On Monday, 3 June 2019 21:21:17 CEST John Crispin wrote: >> The ppet field inside ieee80211_he_capabilities is of size [0]. The >> code currently copies up to 12 additional bytes into the buffer, thus >> overwriting memory. Fix this by verifying the size properly and using >> the passed length value for allocation and the following memcpy() call. >> >> Signed-off-by: John Crispin <john@phrozen.org> > I just went to the patches which might be interesting for me to get HE in a working state for ath11k mesh. And so I found following patches which seemed to be relevant for me: > > * https://patchwork.ozlabs.org/patch/1109462/ (this patched; marked as > "changes requested") > * https://patchwork.ozlabs.org/patch/1122057/ (marked superseded) > > It looks to me that the mentioned patches can all be replaced by https://patchwork.ozlabs.org/patch/1116968/ - is this assumption correct or am I missing something? That is correct, you will also need this patch in your kernel https://patchwork.kernel.org/patch/11019277/ John > [...] >> diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c index >> ba22a174a..63270228f 100644 >> --- a/src/ap/ieee802_11_he.c >> +++ b/src/ap/ieee802_11_he.c >> @@ -44,6 +44,39 @@ static u8 ieee80211_he_ppet_size(u8 ppe_thres_hdr, >> const > u8 *phy_cap_info) >> } >> >> >> +static inline u8 >> +ieee80211_he_mcs_set_size(const u8 *phy_cap_info) { > We talked about this on a call (were I had problems to understand you) and I was under the impression that you were talking about static inline functions in headers. So I have to retract my statement and agree with Jouni about this. > > Kind regards, > Sven > ________________________________ > 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. > ________________________________ > > > _______________________________________________ > Hostap mailing list > Hostap@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/hostap
diff --git a/src/ap/ieee802_11_he.c b/src/ap/ieee802_11_he.c index ba22a174a..63270228f 100644 --- a/src/ap/ieee802_11_he.c +++ b/src/ap/ieee802_11_he.c @@ -44,6 +44,39 @@ static u8 ieee80211_he_ppet_size(u8 ppe_thres_hdr, const u8 *phy_cap_info) } +static inline u8 +ieee80211_he_mcs_set_size(const u8 *phy_cap_info) +{ + u8 sz = 0; + + 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; + if (phy_cap_info[HE_PHYCAP_CHANNEL_WIDTH_SET_IDX] & + (HE_PHYCAP_CHANNEL_WIDTH_SET_40MHZ_IN_2G | HE_PHYCAP_CHANNEL_WIDTH_SET_40MHZ_80MHZ_IN_5G)) + sz += 4; + + return sz; +} + +static inline 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(struct ieee80211_he_capabilities); + + 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) { struct ieee80211_he_capabilities *cap; @@ -322,7 +355,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) || - 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; @@ -331,13 +364,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;
The ppet field inside ieee80211_he_capabilities is of size [0]. The code currently copies up to 12 additional bytes into the buffer, thus overwriting memory. Fix this by verifying the size properly and using the passed length value for allocation and the following memcpy() call. Signed-off-by: John Crispin <john@phrozen.org> --- src/ap/ieee802_11_he.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)