| Message ID | 20191126083917.5995-4-stefan.bader@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 26.11.19 09:39, Stefan Bader wrote: > From: qize wang <wangqize888888888@gmail.com> > > mwifiex_process_tdls_action_frame() without checking > the incoming tdls infomation element's vality before use it, > this may cause multi heap buffer overflows. > > Fix them by putting vality check before use it. > > Signed-off-by: qize wang <wangqize888888888@gmail.com> > > CVE-2019-14901 > > (cherry picked from https://patchwork.kernel.org/patch/11257535/) > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/net/wireless/marvell/mwifiex/tdls.c | 70 +++++++++++++++++++-- > 1 file changed, 64 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c > index 18e654dc34c6..7f60214852c0 100644 > --- a/drivers/net/wireless/marvell/mwifiex/tdls.c > +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c > @@ -954,59 +954,117 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, > > switch (*pos) { > case WLAN_EID_SUPP_RATES: > + if (pos[1] > 32) > + return; > sta_ptr->tdls_cap.rates_len = pos[1]; > for (i = 0; i < pos[1]; i++) > sta_ptr->tdls_cap.rates[i] = pos[i + 2]; > break; > > case WLAN_EID_EXT_SUPP_RATES: > + if (pos[1] > 32) > + return; > basic = sta_ptr->tdls_cap.rates_len; > + if (pos[1] > 32 - basic) > + return; > for (i = 0; i < pos[1]; i++) > sta_ptr->tdls_cap.rates[basic + i] = pos[i + 2]; > sta_ptr->tdls_cap.rates_len += pos[1]; > break; > case WLAN_EID_HT_CAPABILITY: > - memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos, > + if (pos > end - sizeof(struct ieee80211_ht_cap) - 2) > + return; > + if (pos[1] != sizeof(struct ieee80211_ht_cap)) > + return; > + /* copy the ie's value into ht_capb*/ > + memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2, This is changing the memcpy to start the copy from 'pos' to 'pos + 2', but following the discussion on the patchwork link it seems the original code was wrong and it was fixed here but without a comment on the commit message. So if accepted upstream the commit message might get changed. > sizeof(struct ieee80211_ht_cap)); > sta_ptr->is_11n_enabled = 1; > break; > case WLAN_EID_HT_OPERATION: > - memcpy(&sta_ptr->tdls_cap.ht_oper, pos, > + if (pos > end - > + sizeof(struct ieee80211_ht_operation) - 2) > + return; > + if (pos[1] != sizeof(struct ieee80211_ht_operation)) > + return; > + /* copy the ie's value into ht_oper*/ > + memcpy(&sta_ptr->tdls_cap.ht_oper, pos + 2, > sizeof(struct ieee80211_ht_operation)); > break; > case WLAN_EID_BSS_COEX_2040: > + if (pos > end - 3) > + return; > + if (pos[1] != 1) > + return; > sta_ptr->tdls_cap.coex_2040 = pos[2]; > break; > case WLAN_EID_EXT_CAPABILITY: > + if (pos > end - sizeof(struct ieee_types_header)) > + return; > + if (pos[1] < sizeof(struct ieee_types_header)) > + return; > + if (pos[1] > 8) > + return; > memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos, > sizeof(struct ieee_types_header) + > min_t(u8, pos[1], 8)); > break; > case WLAN_EID_RSN: > + if (pos > end - sizeof(struct ieee_types_header)) > + return; > + if (pos[1] < sizeof(struct ieee_types_header)) > + return; > + if (pos[1] > IEEE_MAX_IE_SIZE - > + sizeof(struct ieee_types_header)) > + return; > memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos, > sizeof(struct ieee_types_header) + > min_t(u8, pos[1], IEEE_MAX_IE_SIZE - > sizeof(struct ieee_types_header))); > break; > case WLAN_EID_QOS_CAPA: > + if (pos > end - 3) > + return; > + if (pos[1] != 1) > + return; > sta_ptr->tdls_cap.qos_info = pos[2]; > break; > case WLAN_EID_VHT_OPERATION: > - if (priv->adapter->is_hw_11ac_capable) > - memcpy(&sta_ptr->tdls_cap.vhtoper, pos, > + if (priv->adapter->is_hw_11ac_capable) { > + if (pos > end - > + sizeof(struct ieee80211_vht_operation) - 2) > + return; > + if (pos[1] != > + sizeof(struct ieee80211_vht_operation)) > + return; > + /* copy the ie's value into vhtoper*/ > + memcpy(&sta_ptr->tdls_cap.vhtoper, pos + 2, > sizeof(struct ieee80211_vht_operation)); > + } > break; > case WLAN_EID_VHT_CAPABILITY: > if (priv->adapter->is_hw_11ac_capable) { > - memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos, > + if (pos > end - > + sizeof(struct ieee80211_vht_cap) - 2) > + return; > + if (pos[1] != sizeof(struct ieee80211_vht_cap)) > + return; > + /* copy the ie's value into vhtcap*/ > + memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos + 2, > sizeof(struct ieee80211_vht_cap)); > sta_ptr->is_11ac_enabled = 1; > } > break; > case WLAN_EID_AID: > - if (priv->adapter->is_hw_11ac_capable) > + if (priv->adapter->is_hw_11ac_capable) { > + if (pos > end - 4) > + return; > + if (pos[1] != 2) > + return; > sta_ptr->tdls_cap.aid = > get_unaligned_le16((pos + 2)); > + } > + break; > default: > break; > } >
On 27.11.19 17:21, Kleber Souza wrote: > On 26.11.19 09:39, Stefan Bader wrote: >> From: qize wang <wangqize888888888@gmail.com> >> >> mwifiex_process_tdls_action_frame() without checking >> the incoming tdls infomation element's vality before use it, >> this may cause multi heap buffer overflows. >> >> Fix them by putting vality check before use it. >> >> Signed-off-by: qize wang <wangqize888888888@gmail.com> >> >> CVE-2019-14901 >> >> (cherry picked from https://patchwork.kernel.org/patch/11257535/) >> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> >> --- >> drivers/net/wireless/marvell/mwifiex/tdls.c | 70 +++++++++++++++++++-- >> 1 file changed, 64 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c >> index 18e654dc34c6..7f60214852c0 100644 >> --- a/drivers/net/wireless/marvell/mwifiex/tdls.c >> +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c >> @@ -954,59 +954,117 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, >> >> switch (*pos) { >> case WLAN_EID_SUPP_RATES: >> + if (pos[1] > 32) >> + return; >> sta_ptr->tdls_cap.rates_len = pos[1]; >> for (i = 0; i < pos[1]; i++) >> sta_ptr->tdls_cap.rates[i] = pos[i + 2]; >> break; >> >> case WLAN_EID_EXT_SUPP_RATES: >> + if (pos[1] > 32) >> + return; >> basic = sta_ptr->tdls_cap.rates_len; >> + if (pos[1] > 32 - basic) >> + return; >> for (i = 0; i < pos[1]; i++) >> sta_ptr->tdls_cap.rates[basic + i] = pos[i + 2]; >> sta_ptr->tdls_cap.rates_len += pos[1]; >> break; >> case WLAN_EID_HT_CAPABILITY: >> - memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos, >> + if (pos > end - sizeof(struct ieee80211_ht_cap) - 2) >> + return; >> + if (pos[1] != sizeof(struct ieee80211_ht_cap)) >> + return; >> + /* copy the ie's value into ht_capb*/ >> + memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2, > > This is changing the memcpy to start the copy from 'pos' to 'pos + 2', > but following the discussion on the patchwork link it seems the original > code was wrong and it was fixed here but without a comment on the commit > message. So if accepted upstream the commit message might get changed. What I understood from the upstream discussion there is two different targets of memcpy's. Some are of type header which still need the additional 2 bytes. The others are of a different type which does not contain the header part. > >> sizeof(struct ieee80211_ht_cap)); >> sta_ptr->is_11n_enabled = 1; >> break; >> case WLAN_EID_HT_OPERATION: >> - memcpy(&sta_ptr->tdls_cap.ht_oper, pos, >> + if (pos > end - >> + sizeof(struct ieee80211_ht_operation) - 2) >> + return; >> + if (pos[1] != sizeof(struct ieee80211_ht_operation)) >> + return; >> + /* copy the ie's value into ht_oper*/ >> + memcpy(&sta_ptr->tdls_cap.ht_oper, pos + 2, >> sizeof(struct ieee80211_ht_operation)); >> break; >> case WLAN_EID_BSS_COEX_2040: >> + if (pos > end - 3) >> + return; >> + if (pos[1] != 1) >> + return; >> sta_ptr->tdls_cap.coex_2040 = pos[2]; >> break; >> case WLAN_EID_EXT_CAPABILITY: >> + if (pos > end - sizeof(struct ieee_types_header)) >> + return; >> + if (pos[1] < sizeof(struct ieee_types_header)) >> + return; >> + if (pos[1] > 8) >> + return; >> memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos, >> sizeof(struct ieee_types_header) + >> min_t(u8, pos[1], 8)); >> break; >> case WLAN_EID_RSN: >> + if (pos > end - sizeof(struct ieee_types_header)) >> + return; >> + if (pos[1] < sizeof(struct ieee_types_header)) >> + return; >> + if (pos[1] > IEEE_MAX_IE_SIZE - >> + sizeof(struct ieee_types_header)) >> + return; >> memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos, >> sizeof(struct ieee_types_header) + >> min_t(u8, pos[1], IEEE_MAX_IE_SIZE - >> sizeof(struct ieee_types_header))); >> break; >> case WLAN_EID_QOS_CAPA: >> + if (pos > end - 3) >> + return; >> + if (pos[1] != 1) >> + return; >> sta_ptr->tdls_cap.qos_info = pos[2]; >> break; >> case WLAN_EID_VHT_OPERATION: >> - if (priv->adapter->is_hw_11ac_capable) >> - memcpy(&sta_ptr->tdls_cap.vhtoper, pos, >> + if (priv->adapter->is_hw_11ac_capable) { >> + if (pos > end - >> + sizeof(struct ieee80211_vht_operation) - 2) >> + return; >> + if (pos[1] != >> + sizeof(struct ieee80211_vht_operation)) >> + return; >> + /* copy the ie's value into vhtoper*/ >> + memcpy(&sta_ptr->tdls_cap.vhtoper, pos + 2, >> sizeof(struct ieee80211_vht_operation)); >> + } >> break; >> case WLAN_EID_VHT_CAPABILITY: >> if (priv->adapter->is_hw_11ac_capable) { >> - memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos, >> + if (pos > end - >> + sizeof(struct ieee80211_vht_cap) - 2) >> + return; >> + if (pos[1] != sizeof(struct ieee80211_vht_cap)) >> + return; >> + /* copy the ie's value into vhtcap*/ >> + memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos + 2, >> sizeof(struct ieee80211_vht_cap)); >> sta_ptr->is_11ac_enabled = 1; >> } >> break; >> case WLAN_EID_AID: >> - if (priv->adapter->is_hw_11ac_capable) >> + if (priv->adapter->is_hw_11ac_capable) { >> + if (pos > end - 4) >> + return; >> + if (pos[1] != 2) >> + return; >> sta_ptr->tdls_cap.aid = >> get_unaligned_le16((pos + 2)); >> + } >> + break; >> default: >> break; >> } >> >
diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c index 18e654dc34c6..7f60214852c0 100644 --- a/drivers/net/wireless/marvell/mwifiex/tdls.c +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c @@ -954,59 +954,117 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv, switch (*pos) { case WLAN_EID_SUPP_RATES: + if (pos[1] > 32) + return; sta_ptr->tdls_cap.rates_len = pos[1]; for (i = 0; i < pos[1]; i++) sta_ptr->tdls_cap.rates[i] = pos[i + 2]; break; case WLAN_EID_EXT_SUPP_RATES: + if (pos[1] > 32) + return; basic = sta_ptr->tdls_cap.rates_len; + if (pos[1] > 32 - basic) + return; for (i = 0; i < pos[1]; i++) sta_ptr->tdls_cap.rates[basic + i] = pos[i + 2]; sta_ptr->tdls_cap.rates_len += pos[1]; break; case WLAN_EID_HT_CAPABILITY: - memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos, + if (pos > end - sizeof(struct ieee80211_ht_cap) - 2) + return; + if (pos[1] != sizeof(struct ieee80211_ht_cap)) + return; + /* copy the ie's value into ht_capb*/ + memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2, sizeof(struct ieee80211_ht_cap)); sta_ptr->is_11n_enabled = 1; break; case WLAN_EID_HT_OPERATION: - memcpy(&sta_ptr->tdls_cap.ht_oper, pos, + if (pos > end - + sizeof(struct ieee80211_ht_operation) - 2) + return; + if (pos[1] != sizeof(struct ieee80211_ht_operation)) + return; + /* copy the ie's value into ht_oper*/ + memcpy(&sta_ptr->tdls_cap.ht_oper, pos + 2, sizeof(struct ieee80211_ht_operation)); break; case WLAN_EID_BSS_COEX_2040: + if (pos > end - 3) + return; + if (pos[1] != 1) + return; sta_ptr->tdls_cap.coex_2040 = pos[2]; break; case WLAN_EID_EXT_CAPABILITY: + if (pos > end - sizeof(struct ieee_types_header)) + return; + if (pos[1] < sizeof(struct ieee_types_header)) + return; + if (pos[1] > 8) + return; memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos, sizeof(struct ieee_types_header) + min_t(u8, pos[1], 8)); break; case WLAN_EID_RSN: + if (pos > end - sizeof(struct ieee_types_header)) + return; + if (pos[1] < sizeof(struct ieee_types_header)) + return; + if (pos[1] > IEEE_MAX_IE_SIZE - + sizeof(struct ieee_types_header)) + return; memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos, sizeof(struct ieee_types_header) + min_t(u8, pos[1], IEEE_MAX_IE_SIZE - sizeof(struct ieee_types_header))); break; case WLAN_EID_QOS_CAPA: + if (pos > end - 3) + return; + if (pos[1] != 1) + return; sta_ptr->tdls_cap.qos_info = pos[2]; break; case WLAN_EID_VHT_OPERATION: - if (priv->adapter->is_hw_11ac_capable) - memcpy(&sta_ptr->tdls_cap.vhtoper, pos, + if (priv->adapter->is_hw_11ac_capable) { + if (pos > end - + sizeof(struct ieee80211_vht_operation) - 2) + return; + if (pos[1] != + sizeof(struct ieee80211_vht_operation)) + return; + /* copy the ie's value into vhtoper*/ + memcpy(&sta_ptr->tdls_cap.vhtoper, pos + 2, sizeof(struct ieee80211_vht_operation)); + } break; case WLAN_EID_VHT_CAPABILITY: if (priv->adapter->is_hw_11ac_capable) { - memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos, + if (pos > end - + sizeof(struct ieee80211_vht_cap) - 2) + return; + if (pos[1] != sizeof(struct ieee80211_vht_cap)) + return; + /* copy the ie's value into vhtcap*/ + memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos + 2, sizeof(struct ieee80211_vht_cap)); sta_ptr->is_11ac_enabled = 1; } break; case WLAN_EID_AID: - if (priv->adapter->is_hw_11ac_capable) + if (priv->adapter->is_hw_11ac_capable) { + if (pos > end - 4) + return; + if (pos[1] != 2) + return; sta_ptr->tdls_cap.aid = get_unaligned_le16((pos + 2)); + } + break; default: break; }