[PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Greg KH]

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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Dan Carpenter]

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Dan Carpenter]

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);
}


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Dan Carpenter]

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);
}
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Dan Carpenter]

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'

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Lee]

> 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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Dan Carpenter]

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Lee]

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
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192e: Fix possible buffer overflow in _rtl92e_wx_set_scan

[Dan Carpenter]

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel