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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Date: Tue Aug 23 19:00:42 2011 -0500
From: wlanfae <[email protected]>
[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);
}
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);
}
> 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
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
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
>
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