Skip to contentSkip to conversation

Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

PATCH Superseded Dan Carpenter 3 participants 7 messages Jul 17, 2015 – Jul 19, 2015

Subsystems (2)
Staging Kernel janitors

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 2/2 Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

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 <[email protected]>
drivers/staging/rtl8192e/rtl819x_BAProc.c +1 -1
index 60f536c..98e6c4e 100644
@@ -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);
#1 lore raw
Quoted 12 lines · 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 <[email protected]>
drivers/staging/rtl8192e/rtl819x_BAProc.c +0 -0
index 60f536c..98e6c4e 100644

drivers/staging/rtl8192e/rtl819x_BAProc.c int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb) · line 459

Quoted 8 lines
drivers/staging/rtl8192e/rtl819x_BAProc.c +1 -1
@@ -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.

author
#2 lore raw

PATCH v2 2/2 Staging: rtl8192e: pointer math bug in rtllib_rx_DELBA()

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 <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

---
v2: Mateusz noticed some more pointer math bugs on the next lines.

drivers/staging/rtl8192e/rtl819x_BAProc.c +1 -4
index 60f536c..78ede4a 100644
@@ -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;
#3 lore raw
Quoted 25 lines · 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 <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

---
v2: Mateusz noticed some more pointer math bugs on the next lines.

drivers/staging/rtl8192e/rtl819x_BAProc.c +0 -1
index 60f536c..78ede4a 100644
@@ -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) {

drivers/staging/rtl8192e/rtl819x_BAProc.c int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb) · line 458

Quoted 10 lines
drivers/staging/rtl8192e/rtl819x_BAProc.c +1 -3
@@ -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

author
#5 lore raw
In reply to Dan Carpenter
Quoted 17 lines · Mateusz Kulikowski
Quoted 6 lines
drivers/staging/rtl8192e/rtl819x_BAProc.c +0 -0
index 60f536c..98e6c4e 100644

drivers/staging/rtl8192e/rtl819x_BAProc.c int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb) · line 459

Quoted 8 lines
drivers/staging/rtl8192e/rtl819x_BAProc.c +1 -1
@@ -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,

Quoted 6 lines

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

#6 lore raw
In reply to Dan Carpenter
Quoted 20 lines · Dan Carpenter
Quoted 19 lines · Mateusz Kulikowski
Quoted 6 lines
drivers/staging/rtl8192e/rtl819x_BAProc.c +0 -0
index 60f536c..98e6c4e 100644

drivers/staging/rtl8192e/rtl819x_BAProc.c int rtllib_rx_DELBA(struct rtllib_device *ieee, struct sk_buff *skb) · line 459

Quoted 8 lines
drivers/staging/rtl8192e/rtl819x_BAProc.c +1 -1
@@ -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 :(

Quoted 7 lines
Quoted 4 lines

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