| Message ID | 20191129101054.2756-1-wangqize888888888@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame() | expand |
qize wang <wangqize888888888@gmail.com> writes: > 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. > > IE is TLV struct, but ht_cap and ht_oper aren’t TLV struct. > the origin marvell driver code is wrong: > > memcpy(&sta_ptr->tdls_cap.ht_oper, pos,.... > memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,... > > Fix the bug by changing pos(the address of IE) to > pos+2 ( the address of IE value ). > > v3: change commit log > > Signed-off-by: qize wang <wangqize888888888@gmail.com> Applied manually (removed the changelog from commit), thanks. 1e58252e334d mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()
A bit late, but a few readability and maintainability thoughts: On Fri, Nov 29, 2019 at 2:12 AM 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. > > IE is TLV struct, but ht_cap and ht_oper aren’t TLV struct. > the origin marvell driver code is wrong: > > memcpy(&sta_ptr->tdls_cap.ht_oper, pos,.... > memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,... > > Fix the bug by changing pos(the address of IE) to > pos+2 ( the address of IE value ). > > v3: change commit log > Would have been great to have a Cc: <stable@vger.kernel.org> tag here. I'm not sure if "just have GregKH on CC" is the right process... > Signed-off-by: qize wang <wangqize888888888@gmail.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 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) Really, you needed a magic '32' here? Would be much clearer with: if (pos[1] > sizeof(sta_ptr->tdls_cap.rates)) Same with many of the other cases. > + return; Is 'return' really the right answer for all of these? Just because, e.g., the rates IE is larger than our internal struct, should we really drop the entire frame? Should we be continuing to parse the other IEs, if possible? Or is this overflow a sign of a totally damaged (possibly malicious) frame, because it's required to be smaller than this? (Sorry, I didn't read the spec here yet.) > 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_HT_CAPABILITY: > - memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos, > + if (pos > end - sizeof(struct ieee80211_ht_cap) - 2) > + return; For checks like this ("past 'end'"), it does make sense to return. Brian
On Mon, Dec 02, 2019 at 03:16:35PM -0800, Brian Norris wrote: > A bit late, but a few readability and maintainability thoughts: > > On Fri, Nov 29, 2019 at 2:12 AM 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. > > > > IE is TLV struct, but ht_cap and ht_oper aren’t TLV struct. > > the origin marvell driver code is wrong: > > > > memcpy(&sta_ptr->tdls_cap.ht_oper, pos,.... > > memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,... > > > > Fix the bug by changing pos(the address of IE) to > > pos+2 ( the address of IE value ). > > > > v3: change commit log > > > > Would have been great to have a > > Cc: <stable@vger.kernel.org> > > tag here. I'm not sure if "just have GregKH on CC" is the right process... Not at all :)
On Mon, Dec 02, 2019 at 03:16:35PM -0800, Brian Norris wrote:
> A bit late, but a few readability and maintainability thoughts:
I worded most of my suggestions in the form of a patch here:
[PATCH] mwifiex: drop most magic numbers from mwifiex_process_tdls_action_frame()
https://lore.kernel.org/linux-wireless/20191206194535.150179-1-briannorris@chromium.org/T/#u
https://patchwork.kernel.org/patch/11277011/
Regards,
Brian
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. IE is TLV struct, but ht_cap and ht_oper aren’t TLV struct. the origin marvell driver code is wrong: memcpy(&sta_ptr->tdls_cap.ht_oper, pos,.... memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,... Fix the bug by changing pos(the address of IE) to pos+2 ( the address of IE value ). v3: change commit log Signed-off-by: qize wang <wangqize888888888@gmail.com> --- drivers/net/wireless/marvell/mwifiex/tdls.c | 70 ++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 6 deletions(-)