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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxx>
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@xxxxxxxxxxxx>
Date:   Tue Aug 23 19:00:42 2011 -0500

    From: wlanfae <wlanfae@xxxxxxxxxxx>
    [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);
}