* Re: [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()
2015-07-17 9:24 [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA() Dan Carpenter
@ 2015-07-17 20:17 ` Mateusz Kulikowski
2015-07-19 10:52 ` [patch 2/2 v2] " Dan Carpenter
2015-07-18 7:31 ` [patch 2/2] " Dan Carpenter
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Mateusz Kulikowski @ 2015-07-17 20:17 UTC (permalink / raw)
To: kernel-janitors
n 17.07.2015 11:24, Dan Carpenter wrote:
> The "delba" variable is a pointer to struct rtllib_hdr_3addr so this
> pointer math bug means that we read nonsense data from beyond the end of
> the buffer. It could result in a oops if the memory is not mapped.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/staging/rtl8192e/rtl819x_BAProc.c b/drivers/staging/rtl8192e/rtl819x_BAProc.c
> index 60f536c..98e6c4e 100644
> --- a/drivers/staging/rtl8192e/rtl819x_BAProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_BAProc.c
> @@ -453,7 +453,7 @@ int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb)
> #endif
> delba = (struct rtllib_hdr_3addr *)skb->data;
> dst = (u8 *)(&delba->addr2[0]);
> - delba += sizeof(struct rtllib_hdr_3addr);
> + delba++;
> pDelBaParamSet = (union delba_param_set *)(delba+2);
> pReasonCode = (u16 *)(delba+4);
>
>
Ack/+1,
It's not the last fix for 'delba' unfortunately :(
Next lines should use delba as u8 if I see correctly
See rtllib_DELBA() (line 141) - It first fills skb with rtllib_hdr_3_addr,
and then adds skb delbaparam_set and reasonCode (as a 6 bytes).
Or I'm tired - will re-check tomorrow.
Funniest thing is - it is processed like that since at least 2011..
Good thing is - this code seems not to be executed
at least not on my 'ordinary' test configuration (WPA2, 2.5G N).
I will try to figure out - perhaps it can be safely
removed (conditions for it's execution may be never never met).
Bad thing is - this driver(s) are probably full of bugs like that :/
M.
^ permalink raw reply [flat|nested] 7+ messages in thread* [patch 2/2 v2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()
2015-07-17 20:17 ` Mateusz Kulikowski
@ 2015-07-19 10:52 ` Dan Carpenter
2015-07-19 18:17 ` Mateusz Kulikowski
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2015-07-19 10:52 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Mateusz Kulikowski, Vaishali Thakkar, Mahati Chamarthy, Chen Gang,
Matthew Casey, devel, linux-kernel, kernel-janitors
The pointer math here was totally wrong so we were reading nonsense
information from beyond the end of the buffer. It could lead to an oops
if that memory wasn't mapped.
The "pReasonCode" pointer is assigned but never used so I deleted it.
With-Fix-From: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Mateusz noticed some more pointer math bugs on the next lines.
diff --git a/drivers/staging/rtl8192e/rtl819x_BAProc.c b/drivers/staging/rtl8192e/rtl819x_BAProc.c
index 60f536c..78ede4a 100644
--- a/drivers/staging/rtl8192e/rtl819x_BAProc.c
+++ b/drivers/staging/rtl8192e/rtl819x_BAProc.c
@@ -428,7 +428,6 @@ int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb)
{
struct rtllib_hdr_3addr *delba = NULL;
union delba_param_set *pDelBaParamSet = NULL;
- u16 *pReasonCode = NULL;
u8 *dst = NULL;
if (skb->len < sizeof(struct rtllib_hdr_3addr) + 6) {
@@ -453,9 +452,7 @@ int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb)
#endif
delba = (struct rtllib_hdr_3addr *)skb->data;
dst = (u8 *)(&delba->addr2[0]);
- delba += sizeof(struct rtllib_hdr_3addr);
- pDelBaParamSet = (union delba_param_set *)(delba+2);
- pReasonCode = (u16 *)(delba+4);
+ pDelBaParamSet = (union delba_param_set *)&delba->payload[2];
if (pDelBaParamSet->field.Initiator = 1) {
struct rx_ts_record *pRxTs;
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [patch 2/2 v2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()
2015-07-19 10:52 ` [patch 2/2 v2] " Dan Carpenter
@ 2015-07-19 18:17 ` Mateusz Kulikowski
0 siblings, 0 replies; 7+ messages in thread
From: Mateusz Kulikowski @ 2015-07-19 18:17 UTC (permalink / raw)
To: Dan Carpenter, Greg Kroah-Hartman
Cc: Vaishali Thakkar, Mahati Chamarthy, Chen Gang, Matthew Casey,
devel, linux-kernel, kernel-janitors
On 19.07.2015 12:52, Dan Carpenter wrote:
> The pointer math here was totally wrong so we were reading nonsense
> information from beyond the end of the buffer. It could lead to an oops
> if that memory wasn't mapped.
>
> The "pReasonCode" pointer is assigned but never used so I deleted it.
>
> With-Fix-From: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Mateusz noticed some more pointer math bugs on the next lines.
>
> diff --git a/drivers/staging/rtl8192e/rtl819x_BAProc.c b/drivers/staging/rtl8192e/rtl819x_BAProc.c
> index 60f536c..78ede4a 100644
> --- a/drivers/staging/rtl8192e/rtl819x_BAProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_BAProc.c
> @@ -428,7 +428,6 @@ int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb)
> {
> struct rtllib_hdr_3addr *delba = NULL;
> union delba_param_set *pDelBaParamSet = NULL;
> - u16 *pReasonCode = NULL;
> u8 *dst = NULL;
>
> if (skb->len < sizeof(struct rtllib_hdr_3addr) + 6) {
> @@ -453,9 +452,7 @@ int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb)
> #endif
> delba = (struct rtllib_hdr_3addr *)skb->data;
> dst = (u8 *)(&delba->addr2[0]);
> - delba += sizeof(struct rtllib_hdr_3addr);
> - pDelBaParamSet = (union delba_param_set *)(delba+2);
> - pReasonCode = (u16 *)(delba+4);
> + pDelBaParamSet = (union delba_param_set *)&delba->payload[2];
>
> if (pDelBaParamSet->field.Initiator = 1) {
> struct rx_ts_record *pRxTs;
>
Acked/Tested-by me
Thanks,
Mateusz
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()
2015-07-17 9:24 [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA() Dan Carpenter
2015-07-17 20:17 ` Mateusz Kulikowski
@ 2015-07-18 7:31 ` Dan Carpenter
2015-07-18 8:09 ` Dan Carpenter
2015-07-18 12:32 ` Malcolm Priestley
3 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2015-07-18 7:31 UTC (permalink / raw)
To: kernel-janitors
Thanks, Mateusz. I will send a v2 version of these patches.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()
2015-07-17 9:24 [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA() Dan Carpenter
2015-07-17 20:17 ` Mateusz Kulikowski
2015-07-18 7:31 ` [patch 2/2] " Dan Carpenter
@ 2015-07-18 8:09 ` Dan Carpenter
2015-07-18 12:32 ` Malcolm Priestley
3 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2015-07-18 8:09 UTC (permalink / raw)
To: kernel-janitors
On Fri, Jul 17, 2015 at 10:17:40PM +0200, Mateusz Kulikowski wrote:
> > diff --git a/drivers/staging/rtl8192e/rtl819x_BAProc.c b/drivers/staging/rtl8192e/rtl819x_BAProc.c
> > index 60f536c..98e6c4e 100644
> > --- a/drivers/staging/rtl8192e/rtl819x_BAProc.c
> > +++ b/drivers/staging/rtl8192e/rtl819x_BAProc.c
> > @@ -453,7 +453,7 @@ int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb)
> > #endif
> > delba = (struct rtllib_hdr_3addr *)skb->data;
> > dst = (u8 *)(&delba->addr2[0]);
> > - delba += sizeof(struct rtllib_hdr_3addr);
> > + delba++;
> > pDelBaParamSet = (union delba_param_set *)(delba+2);
> > pReasonCode = (u16 *)(delba+4);
> >
> >
>
> Ack/+1,
>
> It's not the last fix for 'delba' unfortunately :(
>
> Next lines should use delba as u8 if I see correctly
> See rtllib_DELBA() (line 141) - It first fills skb with rtllib_hdr_3_addr,
> and then adds skb delbaparam_set and reasonCode (as a 6 bytes).
> Or I'm tired - will re-check tomorrow.
All this pointer math would have worked if "delba" were a void pointer,
but that's a bit ugly. Anyway, I'll send a proper fix for this.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()
2015-07-17 9:24 [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA() Dan Carpenter
` (2 preceding siblings ...)
2015-07-18 8:09 ` Dan Carpenter
@ 2015-07-18 12:32 ` Malcolm Priestley
3 siblings, 0 replies; 7+ messages in thread
From: Malcolm Priestley @ 2015-07-18 12:32 UTC (permalink / raw)
To: kernel-janitors
On 18/07/15 09:09, Dan Carpenter wrote:
> On Fri, Jul 17, 2015 at 10:17:40PM +0200, Mateusz Kulikowski wrote:
>>> diff --git a/drivers/staging/rtl8192e/rtl819x_BAProc.c b/drivers/staging/rtl8192e/rtl819x_BAProc.c
>>> index 60f536c..98e6c4e 100644
>>> --- a/drivers/staging/rtl8192e/rtl819x_BAProc.c
>>> +++ b/drivers/staging/rtl8192e/rtl819x_BAProc.c
>>> @@ -453,7 +453,7 @@ int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb)
>>> #endif
>>> delba = (struct rtllib_hdr_3addr *)skb->data;
>>> dst = (u8 *)(&delba->addr2[0]);
>>> - delba += sizeof(struct rtllib_hdr_3addr);
>>> + delba++;
>>> pDelBaParamSet = (union delba_param_set *)(delba+2);
>>> pReasonCode = (u16 *)(delba+4);
>>>
>>>
>>
>> Ack/+1,
>>
>> It's not the last fix for 'delba' unfortunately :(
>>
>> Next lines should use delba as u8 if I see correctly
>> See rtllib_DELBA() (line 141) - It first fills skb with rtllib_hdr_3_addr,
>> and then adds skb delbaparam_set and reasonCode (as a 6 bytes).
>> Or I'm tired - will re-check tomorrow.
>
> All this pointer math would have worked if "delba" were a void pointer,
> but that's a bit ugly. Anyway, I'll send a proper fix for this.
Looks like it should be u8.
This is a complete hash of modifying a 3 address header.
However, in kernel struct ieee80211_mgmt has a delba entry to replace
the entire header.
Regards
Malcolm
^ permalink raw reply [flat|nested] 7+ messages in thread