[patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

[patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

[Dan Carpenter]

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

Re: [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

[Mateusz Kulikowski]

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.

Re: [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

[Dan Carpenter]

Thanks, Mateusz.  I will send a v2 version of these patches.

regards,
dan carpenter

Re: [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

[Dan Carpenter]

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

Re: [patch 2/2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

[Malcolm Priestley]

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

[patch 2/2 v2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

[Dan Carpenter]

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;

Re: [patch 2/2 v2] Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

[Mateusz Kulikowski]

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