* [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan @ 2021-02-26 11:48 Lee Gibson 2021-02-26 12:06 ` Greg KH 2021-02-26 13:43 ` Dan Carpenter 0 siblings, 2 replies; 10+ messages in thread From: Lee Gibson @ 2021-02-26 11:48 UTC (permalink / raw) To: gregkh; +Cc: devel, linux-kernel, Lee Gibson Function _rtl92e_wx_set_scan calls memcpy without checking the length. A user could control that length and trigger a buffer overflow. Fix by checking the length is within the maximum allowed size. Signed-off-by: Lee Gibson <leegib@gmail.com> --- drivers/staging/rtl8192e/rtl8192e/rtl_wx.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c index 16bcee13f64b..2acc4f314732 100644 --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c @@ -406,6 +406,9 @@ static int _rtl92e_wx_set_scan(struct net_device *dev, struct iw_scan_req *req = (struct iw_scan_req *)b; if (req->essid_len) { + if (req->essid_len > IW_ESSID_MAX_SIZE) + req->essid_len = IW_ESSID_MAX_SIZE; + ieee->current_network.ssid_len = req->essid_len; memcpy(ieee->current_network.ssid, req->essid, req->essid_len); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-02-26 11:48 [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan Lee Gibson @ 2021-02-26 12:06 ` Greg KH 2021-02-26 12:30 ` Dan Carpenter 2021-02-26 13:43 ` Dan Carpenter 1 sibling, 1 reply; 10+ messages in thread From: Greg KH @ 2021-02-26 12:06 UTC (permalink / raw) To: Lee Gibson; +Cc: devel, linux-kernel On Fri, Feb 26, 2021 at 11:48:29AM +0000, Lee Gibson wrote: > Function _rtl92e_wx_set_scan calls memcpy without checking the length. > A user could control that length and trigger a buffer overflow. > Fix by checking the length is within the maximum allowed size. > > Signed-off-by: Lee Gibson <leegib@gmail.com> > --- > drivers/staging/rtl8192e/rtl8192e/rtl_wx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > index 16bcee13f64b..2acc4f314732 100644 > --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > @@ -406,6 +406,9 @@ static int _rtl92e_wx_set_scan(struct net_device *dev, > struct iw_scan_req *req = (struct iw_scan_req *)b; > > if (req->essid_len) { > + if (req->essid_len > IW_ESSID_MAX_SIZE) > + req->essid_len = IW_ESSID_MAX_SIZE; > + How about using the "pattern" the other wireless drivers use of: int len = min((int)req->essid_len, IW_ESSID_MAX_SIZE); instead? thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-02-26 12:06 ` Greg KH @ 2021-02-26 12:30 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-02-26 12:30 UTC (permalink / raw) To: Greg KH; +Cc: Lee Gibson, devel, linux-kernel On Fri, Feb 26, 2021 at 01:06:46PM +0100, Greg KH wrote: > On Fri, Feb 26, 2021 at 11:48:29AM +0000, Lee Gibson wrote: > > Function _rtl92e_wx_set_scan calls memcpy without checking the length. > > A user could control that length and trigger a buffer overflow. > > Fix by checking the length is within the maximum allowed size. > > > > Signed-off-by: Lee Gibson <leegib@gmail.com> > > --- > > drivers/staging/rtl8192e/rtl8192e/rtl_wx.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > > index 16bcee13f64b..2acc4f314732 100644 > > --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > > +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > > @@ -406,6 +406,9 @@ static int _rtl92e_wx_set_scan(struct net_device *dev, > > struct iw_scan_req *req = (struct iw_scan_req *)b; > > > > if (req->essid_len) { > > + if (req->essid_len > IW_ESSID_MAX_SIZE) > > + req->essid_len = IW_ESSID_MAX_SIZE; > > + > > How about using the "pattern" the other wireless drivers use of: > > int len = min((int)req->essid_len, IW_ESSID_MAX_SIZE); I bet checkpatch complains that we should use min_t() instead (but I'm too lazy to verify). int len = min_t(int, req->essid_len, IW_ESSID_MAX_SIZE); regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-02-26 11:48 [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan Lee Gibson 2021-02-26 12:06 ` Greg KH @ 2021-02-26 13:43 ` Dan Carpenter 2021-02-26 14:05 ` Dan Carpenter 1 sibling, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2021-02-26 13:43 UTC (permalink / raw) To: Lee Gibson; +Cc: gregkh, devel, linux-kernel On Fri, Feb 26, 2021 at 11:48:29AM +0000, Lee Gibson wrote: > Function _rtl92e_wx_set_scan calls memcpy without checking the length. > A user could control that length and trigger a buffer overflow. > Fix by checking the length is within the maximum allowed size. > > Signed-off-by: Lee Gibson <leegib@gmail.com> > --- > drivers/staging/rtl8192e/rtl8192e/rtl_wx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > index 16bcee13f64b..2acc4f314732 100644 > --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c > @@ -406,6 +406,9 @@ static int _rtl92e_wx_set_scan(struct net_device *dev, > struct iw_scan_req *req = (struct iw_scan_req *)b; > > if (req->essid_len) { > + if (req->essid_len > IW_ESSID_MAX_SIZE) > + req->essid_len = IW_ESSID_MAX_SIZE; > + > ieee->current_network.ssid_len = req->essid_len; > memcpy(ieee->current_network.ssid, req->essid, > req->essid_len); > -- Well done. You can add a Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com> to your final patch. How did you spot this bug? It's so ancient that the Fixes tag would look messed up. commit 94a799425eee8225a1e3fbe5f473d2ef04002577 Author: Larry Finger <Larry.Finger@lwfinger.net> Date: Tue Aug 23 19:00:42 2011 -0500 From: wlanfae <wlanfae@realtek.com> [PATCH 1/8] rtl8192e: Import new version of driver from realtek Smatch isn't smart enough to track how this function is called. Smatch tries to track the names of the pointers that a function can be. For example, the pointer is stored in r8192_wx_handlers[] and it's returned from get_handler(). Here is that list. $ smdb.py function_ptr _rtl92e_wx_set_scan _rtl92e_wx_set_scan = ['_rtl92e_wx_set_scan', 'r8192_wx_handlers[]', '(struct iw_handler_def)->standard', 'r get_handler()', 'wireless_process_ioctl ptr handler', 'standard param 4', 'private param 4'] But Smatch gets confused when we do: net/wireless/wext-core.c 951 handler = get_handler(dev, cmd); 952 if (handler) { 953 /* Standard and private are not the same */ 954 if (cmd < SIOCIWFIRSTPRIV) 955 return standard(dev, iwr, cmd, info, handler); Passing the handler pointer to the standard() pointer... 956 else if (private) 957 return private(dev, iwr, cmd, info, handler); 958 } I can hard code the correct function pointer by adding some insert commands into the smatch_data/db/fixup_kernel.sh file. insert into function_ptr values ("fixup_kernel.sh", "r get_handler()", "ioctl_standard_call ptr param 4", 1); insert into function_ptr values ("fixup_kernel.sh", "r get_handler()", "ioctl_standard_iw_point param 3", 1); And now it generates the warning.... But I wonder if probably another idea is to just create a new warning that any time we memcpy() to a (struct ieee80211_network)->ssid and the length is not known to be less than IW_ESSID_MAX_SIZE then print a warning. It turns out this process was slightly more unwieldy than I expected. Adding the types manually seems like it might be a lot of work. Someone could probably go through the list of CVEs from last year and see which types were overflowed. Anyway, I'll test out what I have and post my results next week. regards, dan carpenter #include "smatch.h" #include "smatch_slist.h" #include "smatch_extra.h" static int my_id; static struct { const char *type_name; int len; } member_list[] = { { "(struct ieee80211_network)->ssid", 32 }, { "(struct rtllib_network)->ssid", 32 }, }; static void match_memset(const char *fn, struct expression *expr, void *_unused) { struct expression *dest, *size_expr; struct range_list *rl; char *member_name; int size; int i; dest = get_argument_from_call_expr(expr->args, 0); size_expr = get_argument_from_call_expr(expr->args, 2); if (!dest || !size_expr) return; member_name = get_member_name(dest); if (!member_name) return; for (i = 0; i < ARRAY_SIZE(member_list); i++) { if (strcmp(member_name, member_list[i].type_name) == 0) break; } if (i == ARRAY_SIZE(member_list)) goto free; if (member_list[i].len) size = member_list[i].len; else size = get_array_size_bytes(dest); get_absolute_rl(size_expr, &rl); if (rl_max(rl).value <= size) goto free; sm_msg("protected struct member '%s' overflow: rl='%s'", member_name, show_rl(rl)); free: free_string(member_name); } void check_protected_member(int id) { if (option_project != PROJ_KERNEL) return; my_id = id; add_function_hook("memcpy", &match_memset, NULL); add_function_hook("__memcpy", &match_memset, NULL); } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-02-26 13:43 ` Dan Carpenter @ 2021-02-26 14:05 ` Dan Carpenter 2021-03-01 13:25 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2021-02-26 14:05 UTC (permalink / raw) To: Lee Gibson; +Cc: gregkh, devel, linux-kernel Here is a v2 of my check. I've changed it to mark all "->ssid" and everything in "(struct ieee80211_network)" as protected. I'm just playing around with it at this point to explore what works best. It's impossible to know until after some results come back. regards, dan carpenter #include "smatch.h" #include "smatch_slist.h" #include "smatch_extra.h" static int my_id; static struct { const char *type_name; int len; } member_list[] = { { "(struct ieee80211_network)->ssid", 32 }, { "(struct rtllib_network)->ssid", 32 }, }; static void match_memset(const char *fn, struct expression *expr, void *_unused) { struct expression *dest, *size_arg; struct range_list *rl; char *member_name; int dest_size = 0; int i; dest = get_argument_from_call_expr(expr->args, 0); size_arg = get_argument_from_call_expr(expr->args, 2); if (!dest || !size_arg) return; member_name = get_member_name(dest); if (!member_name) return; for (i = 0; i < ARRAY_SIZE(member_list); i++) { if (strcmp(member_name, member_list[i].type_name) == 0) { dest_size = member_list[i].len; goto check; } } if (strstr(member_name, "->ssid")) goto check; if (strncmp(member_name, "(struct ieee80211_network)", 26) == 0) goto check; goto free; check: get_absolute_rl(size_arg, &rl); if (!dest_size) dest_size = get_array_size_bytes(dest); if (rl_max(rl).value <= dest_size) goto free; if (dest_size <= 0 && is_capped(size_arg)) goto free; sm_msg("protected struct member '%s' overflow: rl='%s'", member_name, show_rl(rl)); free: free_string(member_name); } void check_protected_member(int id) { if (option_project != PROJ_KERNEL) return; my_id = id; add_function_hook("memcpy", &match_memset, NULL); add_function_hook("__memcpy", &match_memset, NULL); } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-02-26 14:05 ` Dan Carpenter @ 2021-03-01 13:25 ` Dan Carpenter 2021-03-01 15:37 ` Lee 2021-03-05 8:22 ` Dan Carpenter 0 siblings, 2 replies; 10+ messages in thread From: Dan Carpenter @ 2021-03-01 13:25 UTC (permalink / raw) To: Lee Gibson; +Cc: devel, gregkh, linux-kernel, linux-wireless On Fri, Feb 26, 2021 at 05:05:26PM +0300, Dan Carpenter wrote: > Here is a v2 of my check. I've changed it to mark all "->ssid" and > everything in "(struct ieee80211_network)" as protected. I'm just > playing around with it at this point to explore what works best. It's > impossible to know until after some results come back. > [ Added linux-wireless to the CC list. Here was the original check I wrote on Friday. https://lore.kernel.org/lkml/20210226140526.GG2222@kadam/ ] This check worked out pretty well. It's probably 50% bugs? Unfiltered results below. The trick of warning for "if (ststr(member, "->ssid")) " and the memcpy length couldn't be verified turned out to be the best. Protecting ieee80211_network didn't find anything. Sometimes we're copying from an existing (presumably verified) config to another config so the ->ssid_len is valid. An example of that is: drivers/staging/rtl8192e/rtllib_softmac.c:1685 rtllib_softmac_new_net() protected struct member '(struct rtllib_network)->ssid' overflow: rl='0-255' drivers/staging/rtl8192e/rtllib_softmac.c 1674 /* Save the essid so that if it is hidden, it is 1675 * replaced with the essid provided by the user. 1676 */ 1677 if (!ssidbroad) { 1678 memcpy(tmp_ssid, ieee->current_network.ssid, 1679 ieee->current_network.ssid_len); 1680 tmp_ssid_len = ieee->current_network.ssid_len; ^^^^^^^^^^^^ We can assume the existing ssid_len is valid 1681 } 1682 memcpy(&ieee->current_network, net, 1683 sizeof(ieee->current_network)); 1684 if (!ssidbroad) { 1685 memcpy(ieee->current_network.ssid, tmp_ssid, 1686 tmp_ssid_len); ^^^^^^^^^^^^ so this memcpy() won't overflow. It's easy enough for a human reviewer to make this sort of assumption, but Smatch can't. 1687 ieee->current_network.ssid_len = tmp_ssid_len; 1688 } All the code outside of drivers/ seems correct. They're mostly similar examples of copying the ssid from one valid existing config to another. The other places are using nla attrs like this: net/wireless/nl80211.c 14399 if (info->attrs[NL80211_ATTR_SSID]) { 14400 params.ssid.ssid_len = nla_len(info->attrs[NL80211_ATTR_SSID]); 14401 if (params.ssid.ssid_len == 0) 14402 return -EINVAL; 14403 memcpy(params.ssid.ssid, 14404 nla_data(info->attrs[NL80211_ATTR_SSID]), 14405 params.ssid.ssid_len); 14406 } Smatch doesn't parse nla attributes correctly. They're capped using the nl80211_policy[] array: net/wireless/nl80211.c 528 [NL80211_ATTR_SSID] = { .type = NLA_BINARY, 529 .len = IEEE80211_MAX_SSID_LEN }, But there are quite a few real bugs as well. If anyone wants to fix any of these just claim a bug, and I won't send a patch for that warning. :) Lee, I think you mentioned that you had found a related buffer overflow fix? Did the check find it? regards, dan carpenter drivers/staging/rtl8192u/r8192U_wx.c:335 r8192_wx_set_scan() protected struct member '(struct ieee80211_network)->ssid' overflow: rl='1-255' drivers/staging/rtl8192e/rtllib_softmac.c:1685 rtllib_softmac_new_net() protected struct member '(struct rtllib_network)->ssid' overflow: rl='0-255' drivers/staging/rtl8192e/rtl8192e/rtl_wx.c:412 _rtl92e_wx_set_scan() protected struct member '(struct rtllib_network)->ssid' overflow: rl='1-255' drivers/staging/rtl8188eu/core/rtw_ap.c:795 rtw_check_beacon_data() protected struct member '(struct ndis_802_11_ssid)->ssid' overflow: rl='1-255' drivers/staging/rtl8188eu/os_dep/ioctl_linux.c:1138 rtw_wx_set_scan() protected struct member '(struct ndis_802_11_ssid)->ssid' overflow: rl='1-127' drivers/net/wireless/ath/wcn36xx/smd.c:1571 wcn36xx_smd_set_bss_params() protected struct member '(struct wcn36xx_hal_mac_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/ath/ath11k/wmi.c:2148 ath11k_wmi_send_scan_start_cmd() protected struct member '(struct wmi_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/ath/wil6210/cfg80211.c:2100 wil_cfg80211_change_beacon() protected struct member '(struct wil6210_vif)->ssid' overflow: rl='0-255' drivers/net/wireless/ath/ath10k/wow.c:198 ath10k_wmi_pno_check() protected struct member '(struct wmi_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/ath/ath10k/wmi-tlv.c:2029 ath10k_wmi_tlv_op_gen_start_scan() protected struct member '(struct wmi_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/ath/ath10k/wmi-tlv.c:3929 ath10k_wmi_tlv_op_gen_config_pno_start() protected struct member '(struct wmi_ssid)->ssid' overflow: rl='0-u32max' drivers/net/wireless/ath/ath6kl/wmi.c:1884 ath6kl_wmi_connect_cmd() protected struct member '(struct wmi_connect_cmd)->ssid' overflow: rl='s32min-(-1),1-s32max' drivers/net/wireless/ath/ath6kl/cfg80211.c:932 ath6kl_set_probed_ssids() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/ath/ath6kl/cfg80211.c:971 ath6kl_set_probed_ssids() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/ath/ath6kl/cfg80211.c:1631 ath6kl_cfg80211_join_ibss() protected struct member '(struct ath6kl_vif)->ssid' overflow: rl='0-255' drivers/net/wireless/ath/ath6kl/cfg80211.c:2809 ath6kl_start_ap() protected struct member '(struct ath6kl_vif)->ssid' overflow: rl='0-65531' drivers/net/wireless/ath/ath6kl/cfg80211.c:2892 ath6kl_start_ap() protected struct member '(struct wmi_connect_cmd)->ssid' overflow: rl='0-65531' drivers/net/wireless/st/cw1200/sta.c:1293 cw1200_do_join() protected struct member '(struct wsm_join)->ssid' overflow: rl='0-255' drivers/net/wireless/st/cw1200/sta.c:2334 cw1200_start_ap() protected struct member '(struct wsm_start)->ssid' overflow: rl='0-255' drivers/net/wireless/quantenna/qtnfmac/event.c:573 qtnf_event_handle_external_auth() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='1-255' drivers/net/wireless/marvell/mwifiex/uap_cmd.c:506 mwifiex_uap_bss_param_prepare() protected struct member '(struct host_cmd_tlv_ssid)->ssid' overflow: rl='1-u32max' drivers/net/wireless/marvell/mwifiex/join.c:428 mwifiex_cmd_802_11_associate() protected struct member '(struct mwifiex_ie_types_ssid_param_set)->ssid' overflow: rl='0-u16max' drivers/net/wireless/marvell/mwifiex/scan.c:933 mwifiex_config_scan() protected struct member '(struct mwifiex_ie_types_wildcard_ssid_params)->ssid' overflow: rl='0-255' drivers/net/wireless/marvell/mwifiex/scan.c:2383 mwifiex_cmd_802_11_bg_scan_config() protected struct member '(struct mwifiex_ie_types_wildcard_ssid_params)->ssid' overflow: rl='0-255' drivers/net/wireless/marvell/mwifiex/cfg80211.c:1999 mwifiex_cfg80211_start_ap() protected struct member '(struct mwifiex_802_11_ssid)->ssid' overflow: rl='1-65531' drivers/net/wireless/marvell/libertas/cfg.c:176 lbs_add_ssid_tlv() protected struct member '(struct mrvl_ie_ssid_param_set)->ssid' overflow: rl='s32min-s32max' drivers/net/wireless/marvell/libertas/cfg.c:1767 lbs_ibss_join_existing() protected struct member '(struct adhoc_bssdesc)->ssid' overflow: rl='0-255' drivers/net/wireless/marvell/libertas/cfg.c:1878 lbs_ibss_start_new() protected struct member '(struct cmd_ds_802_11_ad_hoc_start)->ssid' overflow: rl='0-255' drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3708 brcmf_wowl_nd_results() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/intel/iwlwifi/mvm/d3.c:1932 iwl_mvm_query_netdetect_reasons() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/intel/iwlwifi/mvm/scan.c:486 iwl_scan_build_ssids() protected struct member '(struct iwl_ssid_ie)->ssid' overflow: rl='1-255' drivers/net/wireless/intel/iwlwifi/mvm/scan.c:500 iwl_scan_build_ssids() protected struct member '(struct iwl_ssid_ie)->ssid' overflow: rl='0-255' drivers/net/wireless/intel/iwlwifi/dvm/scan.c:721 iwlagn_request_scan() protected struct member '(struct iwl_ssid_ie)->ssid' overflow: rl='0-255' drivers/net/wireless/intel/ipw2x00/ipw2200.c:5870 ipw_adhoc_create() protected struct member '(struct libipw_network)->ssid' overflow: rl='0-255' drivers/net/wireless/intel/iwlegacy/3945-mac.c:2573 il3945_request_scan() protected struct member '(struct il_ssid_ie)->ssid' overflow: rl='1-255' drivers/net/wireless/intel/iwlegacy/4965-mac.c:919 il4965_request_scan() protected struct member '(struct il_ssid_ie)->ssid' overflow: rl='1-255' drivers/net/wireless/ti/wlcore/scan.c:351 wlcore_scan() protected struct member '(struct wl1271_scan)->ssid' overflow: rl='1-255' drivers/net/wireless/ti/wlcore/scan.c:410 wlcore_scan_sched_scan_ssid_list() protected struct member '(struct wl1271_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/ti/wlcore/scan.c:425 wlcore_scan_sched_scan_ssid_list() protected struct member '(struct wl1271_ssid)->ssid' overflow: rl='1-255' drivers/net/wireless/ti/wl18xx/scan.c:94 wl18xx_scan_send() protected struct member '(struct wl18xx_cmd_scan_params)->ssid' overflow: rl='0-255' drivers/net/wireless/ti/wl1251/cmd.c:459 wl1251_cmd_scan() protected struct member '(struct wl1251_scan_parameters)->ssid' overflow: rl='0-255' drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c:1338 mt76_connac_mcu_hw_scan() protected struct member '(struct mt76_connac_mcu_scan_ssid)->ssid' overflow: rl='1-255' drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c:1445 mt76_connac_mcu_sched_scan_req() protected struct member '(struct mt76_connac_mcu_scan_ssid)->ssid' overflow: rl='0-255' drivers/net/wireless/mediatek/mt76/mt76_connac_mcu.c:1452 mt76_connac_mcu_sched_scan_req() protected struct member '(struct mt76_connac_mcu_scan_match)->ssid' overflow: rl='0-255' net/wireless/nl80211.c:3890 nl80211_set_interface() protected struct member '(struct wireless_dev)->ssid' overflow: rl='0-255' net/wireless/nl80211.c:4001 nl80211_new_interface() protected struct member '(struct wireless_dev)->ssid' overflow: rl='0-255' net/wireless/nl80211.c:5503 nl80211_start_ap() protected struct member '(struct wireless_dev)->ssid' overflow: rl='0-255' net/wireless/nl80211.c:8382 nl80211_trigger_scan() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='0-65527' net/wireless/nl80211.c:8836 nl80211_parse_sched_scan() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='0-65527' net/wireless/nl80211.c:8874 nl80211_parse_sched_scan() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='0-65531' net/wireless/nl80211.c:14403 nl80211_external_auth() protected struct member '(struct cfg80211_ssid)->ssid' overflow: rl='1-255' net/wireless/ibss.c:151 __cfg80211_join_ibss() protected struct member '(struct wireless_dev)->ssid' overflow: rl='0-255' net/wireless/ibss.c:424 cfg80211_ibss_wext_siwessid() protected struct member '(struct wireless_dev)->ssid' overflow: rl='0-u16max' net/wireless/mesh.c:212 __cfg80211_join_mesh() protected struct member '(struct wireless_dev)->ssid' overflow: rl='1-255' net/mac80211/ibss.c:330 __ieee80211_sta_join_ibss() protected struct member '(struct ieee80211_bss_conf)->ssid' overflow: rl='0-255' net/mac80211/ibss.c:1833 ieee80211_ibss_join() protected struct member '(struct ieee80211_if_ibss)->ssid' overflow: rl='0-255' net/mac80211/cfg.c:1132 ieee80211_start_ap() protected struct member '(struct ieee80211_bss_conf)->ssid' overflow: rl='1-65531' ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-03-01 13:25 ` Dan Carpenter @ 2021-03-01 15:37 ` Lee 2021-03-05 8:22 ` Dan Carpenter 1 sibling, 0 replies; 10+ messages in thread From: Lee @ 2021-03-01 15:37 UTC (permalink / raw) To: Dan Carpenter; +Cc: devel, gregkh, linux-kernel, linux-wireless > This check worked out pretty well. It's probably 50% bugs? Unfiltered > results below. The trick of warning for "if (ststr(member, "->ssid")) " > and the memcpy length couldn't be verified turned out to be the best. That list looks great. I checked out 2 of those listed at random and they look like valid bugs to me. > But there are quite a few real bugs as well. If anyone wants to fix any > of these just claim a bug, and I won't send a patch for that warning. > :) Lee, I think you mentioned that you had found a related buffer > overflow fix? Did the check find it? I think I found 2 in these files: drivers/staging/rtl8712/rtl871x_cmd.c drivers/staging/wfx/hif_tx.c Regards, Lee ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-03-01 13:25 ` Dan Carpenter 2021-03-01 15:37 ` Lee @ 2021-03-05 8:22 ` Dan Carpenter 2021-03-05 15:00 ` Lee 1 sibling, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2021-03-05 8:22 UTC (permalink / raw) To: Lee Gibson; +Cc: devel, gregkh, linux-kernel, linux-wireless Actually, I looked through a bunch of these and they're mostly false positives outside of staging. I guess there are a few ways the ->ssid can be changed. Via netlink, from the network or from the an ioctl. I still have a couple questions, but so far as I can see it's mostly the ioctl which has problems. I really want Smatch to be able to figure the netlink stuff... That should be doable. regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-03-05 8:22 ` Dan Carpenter @ 2021-03-05 15:00 ` Lee 2021-03-08 7:57 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Lee @ 2021-03-05 15:00 UTC (permalink / raw) To: Dan Carpenter; +Cc: devel, gregkh, linux-kernel, linux-wireless Hi Dan, Do you think any of these could be potential issues: driver/staging/ rtl8192e/rtllib_rx.c:2442 wlan-ng/cfg80211.c:316 rtl8723bs/os_dep/ioctl_cfg80211.c:1591 rtl8723bs/os_dep/ioctl_cfg80211.c:2738 and if so, findable via Smatch? Regards, Lee On Fri, Mar 05, 2021 at 11:22:28AM +0300, Dan Carpenter wrote: > Actually, I looked through a bunch of these and they're mostly false > positives outside of staging. I guess there are a few ways the ->ssid > can be changed. Via netlink, from the network or from the an ioctl. > > I still have a couple questions, but so far as I can see it's mostly the > ioctl which has problems. > > I really want Smatch to be able to figure the netlink stuff... That > should be doable. > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan 2021-03-05 15:00 ` Lee @ 2021-03-08 7:57 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-03-08 7:57 UTC (permalink / raw) To: Lee; +Cc: devel, gregkh, linux-kernel, linux-wireless On Fri, Mar 05, 2021 at 03:00:14PM +0000, Lee wrote: > > Hi Dan, > > Do you think any of these could be potential issues: > > driver/staging/ > > rtl8192e/rtllib_rx.c:2442 memcpy(dst->ssid, src->ssid, src->ssid_len); Smatch says that at this point we know "src->ssid_len" is in the 1-32 range. This is without any fixes to how Smatch parses nl_len(). > wlan-ng/cfg80211.c:316 313 if (request->n_ssids > 0) { 314 msg1.scantype.data = P80211ENUM_scantype_active; 315 msg1.ssid.data.len = request->ssids->ssid_len; 316 memcpy(msg1.ssid.data.data, 317 request->ssids->ssid, request->ssids->ssid_len); 318 } else { The only thing Smatch knows about "request->ssids->ssid_len" is that it's 0-255. I had not marked "msg1.ssid.data.data" as a protected struct member so it didn't generate a warning. I think cfg80211_scan_request structs are filled out in a systematic way in ieee80211_request_ibss_scan() and they're bounds checked properly so this isn't a bug. > rtl8723bs/os_dep/ioctl_cfg80211.c:1591 > rtl8723bs/os_dep/ioctl_cfg80211.c:2738 Same. regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-08 7:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-26 11:48 [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan Lee Gibson 2021-02-26 12:06 ` Greg KH 2021-02-26 12:30 ` Dan Carpenter 2021-02-26 13:43 ` Dan Carpenter 2021-02-26 14:05 ` Dan Carpenter 2021-03-01 13:25 ` Dan Carpenter 2021-03-01 15:37 ` Lee 2021-03-05 8:22 ` Dan Carpenter 2021-03-05 15:00 ` Lee 2021-03-08 7:57 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).