| Message ID | E40E893E-D9B4-4C63-8139-1DD5E1C2CECB@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame() | expand |
On Fri, Nov 22, 2019 at 05:43:49PM +0800, qize wang wrote: > 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, ^^^^^^^ I don't understand why we changed "pos" to "pos + 2". Presumably there is a reason, but it needs to explained in the commit message. > 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; I was confused by the "- 2" as well in the earlier versions of this patch but Marvell approved it so I assumed it was correct. It would be nice if this patch were tested. regards, dan carpenter
Hi Dan, > > + /* copy the ie's value into ht_capb*/ > > + memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2, > ^^^^^^^ > > I don't understand why we changed "pos" to "pos + 2". Presumably there is > a reason, but it needs to explained in the commit message. I think, we were doing wrong in the original code. We are supposed to use 'pos + 2' itself, instead of just 'pos'. This is because, 'pos' is pointing to 'ieee_types_header', followed by the actual data and the destination do not start with (i.e. it do not contain) 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.ht_oper'). Also, there are few places were the destination starts with 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.extcap'), which need just 'pos'. Regards, Ganapathi
On Fri, Nov 22, 2019 at 11:40:40AM +0000, Ganapathi Bhat wrote: > Hi Dan, > > > > + /* copy the ie's value into ht_capb*/ > > > + memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2, > > ^^^^^^^ > > > > I don't understand why we changed "pos" to "pos + 2". Presumably there is > > a reason, but it needs to explained in the commit message. > > I think, we were doing wrong in the original code. We are supposed to use 'pos + 2' itself, instead of just 'pos'. This is because, 'pos' is pointing to 'ieee_types_header', followed by the actual data and the destination do not start with (i.e. it do not contain) 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.ht_oper'). > > Also, there are few places were the destination starts with 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.extcap'), which need just 'pos'. I assumed it was something like this but it needs to be explained in the commit message. regards, dan carpenter
Hi, dan I forget to explain the reason of changing "pos" to "pos + 2” in the commit message. Thank you for your suggestion. IE is TLV struct, but ht_cap and ht_oper aren’t TLV struct,the origin marvell driver code is wrong. we fix the bug by changing pos(the address of IE) to pos+2 ( the address of IE’s value ). regards, qize wang > 在 2019年11月22日，下午8:37，Dan Carpenter <dan.carpenter@oracle.com> 写道： > > On Fri, Nov 22, 2019 at 11:40:40AM +0000, Ganapathi Bhat wrote: >> Hi Dan, >> >>>> + /* copy the ie's value into ht_capb*/ >>>> + memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2, >>> ^^^^^^^ >>> >>> I don't understand why we changed "pos" to "pos + 2". Presumably there is >>> a reason, but it needs to explained in the commit message. >> >> I think, we were doing wrong in the original code. We are supposed to use 'pos + 2' itself, instead of just 'pos'. This is because, 'pos' is pointing to 'ieee_types_header', followed by the actual data and the destination do not start with (i.e. it do not contain) 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.ht_oper'). >> >> Also, there are few places were the destination starts with 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.extcap'), which need just 'pos'. > > I assumed it was something like this but it needs to be explained in > the commit message. > > regards, > dan carpenter >
This fix addresses CVE-2019-14901 > 在 2019年11月22日，下午10:27，qize wang <wangqize888888888@gmail.com> 写道： > > Hi, dan > > I forget to explain the reason of changing "pos" to "pos + 2” in the commit message. > Thank you for your suggestion. > IE is TLV struct, but ht_cap and ht_oper aren’t TLV struct,the origin marvell driver code is wrong. > we fix the bug by changing pos(the address of IE) to pos+2 ( the address of IE’s value ). > > regards, > qize wang > >> 在 2019年11月22日，下午8:37，Dan Carpenter <dan.carpenter@oracle.com> 写道： >> >> On Fri, Nov 22, 2019 at 11:40:40AM +0000, Ganapathi Bhat wrote: >>> Hi Dan, >>> >>>>> + /* copy the ie's value into ht_capb*/ >>>>> + memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2, >>>> ^^^^^^^ >>>> >>>> I don't understand why we changed "pos" to "pos + 2". Presumably there is >>>> a reason, but it needs to explained in the commit message. >>> >>> I think, we were doing wrong in the original code. We are supposed to use 'pos + 2' itself, instead of just 'pos'. This is because, 'pos' is pointing to 'ieee_types_header', followed by the actual data and the destination do not start with (i.e. it do not contain) 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.ht_oper'). >>> >>> Also, there are few places were the destination starts with 'ieee_types_header'(ex: 'sta_ptr->tdls_cap.extcap'), which need just 'pos'. >> >> I assumed it was something like this but it needs to be explained in >> the commit message. >> >> regards, >> dan carpenter >> >
qize wang <wangqize888888888@gmail.com> wrote: > 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> Please fix the commit log as explained by Dan. Patch set to Changes Requested.
(adding back linux-wireless) Hi qize, qize wang <wangqize888888888@gmail.com> writes: > sorry, I am a new guy. Welcome to the Linux community! But most important thing first: please don't send email privately, instead post any questions you have to the list. Normally I don't reply to private email (I get way too much mail), but making an exception this time as you are new. > I have some question: Should I need to submit a new version patch to > linux-wireless( like [patch v2]....) or reply a comment in the old > patch path. (https://patchwork.kernel.org/patch/11257535/) ? Yes, please submit v2 and include a change log what you changed. More info in the link below.
diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c index 09313047beed..7caf1d26124a 100644 --- a/drivers/net/wireless/marvell/mwifiex/tdls.c +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c @@ -953,59 +953,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; }
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> --- drivers/net/wireless/marvell/mwifiex/tdls.c | 70 ++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 6 deletions(-)