[PATCH] staging: rtl8712: Fix possible buffer overrun
From: Young Xiao
In commit 8b7a13c3f404 ("staging: r8712u: Fix possible buffer
overrun") we fix a potential off by one by making the limit smaller.
The better fix is to make the buffer larger. This makes it match up
with the similar code in other drivers.
Signed-off-by: Young Xiao
---
drivers/staging/rtl8712/mlme_linux.c | 2 +-
drivers/staging/rtl8712/rtl871x_mlme.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8712/mlme_linux.c
b/drivers/staging/rtl8712/mlme_linux.c
index 9d156ef..4d473f0 100644
--- a/drivers/staging/rtl8712/mlme_linux.c
+++ b/drivers/staging/rtl8712/mlme_linux.c
@@ -146,7 +146,7 @@ void r8712_report_sec_ie(struct _adapter *adapter, u8
authmode, u8 *sec_ie)
p = buff;
p += sprintf(p, "ASSOCINFO(ReqIEs=");
len = sec_ie[1] + 2;
- len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX - 1;
+ len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX;
for (i = 0; i < len; i++)
p += sprintf(p, "%02x", sec_ie[i]);
p += sprintf(p, ")");
diff --git a/drivers/staging/rtl8712/rtl871x_mlme.c
b/drivers/staging/rtl8712/rtl871x_mlme.c
index a737400..986a1d5 100644
--- a/drivers/staging/rtl8712/rtl871x_mlme.c
+++ b/drivers/staging/rtl8712/rtl871x_mlme.c
@@ -1346,7 +1346,7 @@ sint r8712_restruct_sec_ie(struct _adapter *adapter, u8
*in_ie,
u8 *out_ie, uint in_len)
{
u8 authmode = 0, match;
- u8 sec_ie[255], uncst_oui[4], bkup_ie[255];
+ u8 sec_ie[IW_CUSTOM_MAX], uncst_oui[4], bkup_ie[255];
u8 wpa_oui[4] = {0x0, 0x50, 0xf2, 0x01};
uint ielength, cnt, remove_cnt;
int iEntry;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: Fix possible buffer overrun
Yes, you are right. I will send a new patch.
Young
On 2018/11/28 14:51, Dan Carpenter wrote:
> The original code is OK.
>
> On Wed, Nov 28, 2018 at 02:22:31AM +0000, Yang Xiao wrote:
>> From: Young Xiao
>>
>> In routine rtw_report_sec_ie(), the code could set the length
>> of the buffer to 256; however, that value is one larger than the
>> corresponding memory allocation.
>>
>> See commit 8b7a13c3f404 ("staging: r8712u: Fix possible
>> buffer overrun") for detail.
> This bug is from 2012... It's a real bug, but looking at things in
> retrospect we probably didn't do the right fix. The correct patch would
> be to revert 8b7a13c3f404 and change this instead:
>
> Can you send that? Do it as one patch. (Don't make it a revert commit,
> that's just a headache, make it a normal patch with a Fixes tag). The
> commit message would look something like:
>
>In commit 8b7a13c3f404 ("staging: r8712u: Fix possible buffer
>overrun") we fix a potential off by one by making the limit smaller.
>The better fix is to make the buffer larger. This makes it match up
>with the similar code in other drivers. Blah blah blah. Etc.
>
> diff --git a/drivers/staging/rtl8712/rtl871x_mlme.c
> b/drivers/staging/rtl8712/rtl871x_mlme.c
> index a7374006a9fb..986a1d526918 100644
> --- a/drivers/staging/rtl8712/rtl871x_mlme.c
> +++ b/drivers/staging/rtl8712/rtl871x_mlme.c
> @@ -1346,7 +1346,7 @@ sint r8712_restruct_sec_ie(struct _adapter *adapter, u8
> *in_ie,
>u8 *out_ie, uint in_len)
> {
> u8 authmode = 0, match;
> - u8 sec_ie[255], uncst_oui[4], bkup_ie[255];
> + u8 sec_ie[IW_CUSTOM_MAX], uncst_oui[4], bkup_ie[255];
> u8 wpa_oui[4] = {0x0, 0x50, 0xf2, 0x01};
> uint ielength, cnt, remove_cnt;
> int iEntry;
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8723bs: Fix possible buffer overrun
From: Young Xiao
In routine rtw_report_sec_ie(), the code could set the length
of the buffer to 256; however, that value is one larger than the
corresponding memory allocation.
See commit 8b7a13c3f404 ("staging: r8712u: Fix possible
buffer overrun") for detail.
Signed-off-by: Young Xiao
---
drivers/staging/rtl8723bs/os_dep/mlme_linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
b/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
index da4bd52..085026c 100644
--- a/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
@@ -165,7 +165,7 @@ void rtw_report_sec_ie(struct adapter *adapter, u8
authmode, u8 *sec_ie)
p += sprintf(p, "ASSOCINFO(ReqIEs =");
len = sec_ie[1] + 2;
- len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX;
+ len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX - 1;
for (i = 0; i < len; i++) {
p += sprintf(p, "%02x", sec_ie[i]);
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Revert commit ef9209b642f "staging: rtl8723bs: Fix indenting errors and an off-by-one mistake in core/rtw_mlme_ext.c"
From: Young Xiao
pstapriv->max_num_sta is always <= NUM_STA, since max_num_sta is either
set in _rtw_init_sta_priv() or rtw_set_beacon().
Signed-off-by: Young Xiao
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 69c7abc..8445d51 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1565,7 +1565,7 @@ unsigned int OnAssocReq(struct adapter *padapter, union
recv_frame *precv_frame)
if (pstat->aid > 0) {
DBG_871X(" old AID %d\n", pstat->aid);
} else {
- for (pstat->aid = 1; pstat->aid < NUM_STA; pstat->aid++)
+ for (pstat->aid = 1; pstat->aid <= NUM_STA; pstat->aid++)
if (pstapriv->sta_aid[pstat->aid - 1] == NULL)
break;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
Yes, you are right. I will send a patch to revert ef9209b642f. Young On 2018/11/27 16:49, Dan Carpenter wrote: > On Tue, Nov 27, 2018 at 08:41:53AM +0000, Yang Xiao wrote: >> Okay. I can send a patch to revert ef9209b642f. >> >> But, can you make sure that the condition "(pstapriv->sta_aid[pstat->aid >> - 1] == NULL)" can satisfies in the for loop? > ->max_num_sta is either set in _rtw_init_sta_priv() or rtw_set_beacon(). > You can see that it's <= MAX_STA. > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
Okay. I can send a patch to revert ef9209b642f.
But, can you make sure that the condition "(pstapriv->sta_aid[pstat->aid
- 1] == NULL)" can satisfies in the for loop?
Young
On 2018/11/27 16:34, Dan Carpenter wrote:
> On Tue, Nov 27, 2018 at 08:29:05AM +, Yang Xiao wrote:
>> Hi,
>>
>> See commit ef9209b642f ("staging: rtl8723bs: Fix indenting errors and an
>> off-by-one mistake in core/rtw_mlme_ext.c") for detail.
>>
>> I don't know how can you make sure that line 3254 can be true in the for
>> loop. If the condition never satisfies, then there is an off-by-one
>> access in line 3267.
>>
>> If you can prove it, then the patch is unnecessary.
>>
> Ugh... Patch ef9209b642f isn't right. :(
>
> Do you want to send a patch to change that back to the way it was. If
> not then I can do it.
>
> regards,
> dan carpenter
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
Hi,
See commit ef9209b642f ("staging: rtl8723bs: Fix indenting errors and an
off-by-one mistake in core/rtw_mlme_ext.c") for detail.
I don't know how can you make sure that line 3254 can be true in the for
loop. If the condition never satisfies, then there is an off-by-one
access in line 3267.
If you can prove it, then the patch is unnecessary.
Young
On 2018/11/27 16:15, Dan Carpenter wrote:
> The original code is OK.
>
> On Tue, Nov 27, 2018 at 07:29:07AM +, Yang Xiao wrote:
>> From: Young_X
>>
>> The error at line 3267 was the result of an off-by-one error in
>> a for loop in line 3253.
>> If condition in line 3254 never satisfies, then the value of
>> pstat->aid is NUM_STA+1. This will lead to out-of-bound access
>> in line 3267.
> It's best to avoid line numbers in the commit message unless you paste
> the actual code, because sometimes people will be using different line
> numbers from you.
>
>> Signed-off-by: Young_X
>> ---
>> drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> index 6790b840..0854adc 100644
>> --- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> +++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>> @@ -3250,7 +3250,7 @@ static unsigned int OnAssocReq(struct adapter
>> *padapter,
>> if (pstat->aid > 0) {
>> DBG_88E(" old AID %d\n", pstat->aid);
>> } else {
>> -for (pstat->aid = 1; pstat->aid <= NUM_STA; pstat->aid++)
>> +for (pstat->aid = 1; pstat->aid < NUM_STA; pstat->aid++)
> drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
>3249 /* get a unique AID */
>3250 if (pstat->aid > 0) {
>3251 DBG_88E(" old AID %d\n", pstat->aid);
>3252 } else {
>3253 for (pstat->aid = 1; pstat->aid <= NUM_STA;
> pstat->aid++)
>3254 if (pstapriv->sta_aid[pstat->aid - 1] ==
> NULL)
>3255 break;
>3256
>3257 /* if (pstat->aid > NUM_STA) { */
>
> This comment is key. pstapriv->max_num_sta is always less than or equal
> to NUM_STA.
>
>3258 if (pstat->aid > pstapriv->max_num_sta) {
>3259 pstat->aid = 0;
>3260
>3261 DBG_88E(" no room for more AIDs\n");
>3262
>3263 status =
> WLAN_STATUS_AP_UNABLE_TO_HANDLE_NEW_STA;
>3264
>3265 goto OnAssocReqFail;
>3266 } else {
>3267 pstapriv->sta_aid[pstat->aid - 1] = pstat;
>^^
> So this is fine.
>
>3268 DBG_88E("allocate new AID=(%d)\n",
> pstat->aid);
>3269 }
>3270 }
>3271
>
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8188eu: Fix off-by-one in core/rtw_mlme_ext.c
From: Young_X
The error at line 3267 was the result of an off-by-one error in
a for loop in line 3253.
If condition in line 3254 never satisfies, then the value of
pstat->aid is NUM_STA+1. This will lead to out-of-bound access
in line 3267.
Signed-off-by: Young_X
---
drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index 6790b840..0854adc 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -3250,7 +3250,7 @@ static unsigned int OnAssocReq(struct adapter *padapter,
if (pstat->aid > 0) {
DBG_88E(" old AID %d\n", pstat->aid);
} else {
- for (pstat->aid = 1; pstat->aid <= NUM_STA; pstat->aid++)
+ for (pstat->aid = 1; pstat->aid < NUM_STA; pstat->aid++)
if (pstapriv->sta_aid[pstat->aid - 1] == NULL)
break;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
