Skip to contentSkip to conversation

staging:rtl8192u: Check memory allocation

PATCH Superseded Georgiana Rodica Chelu 2 participants 2 messages Mar 1, 2017 Staging

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Georgiana Rodica Chelu

author
#0 lore raw

PATCH v2 1/1 staging:rtl8192u: Check memory allocation

Check if the allocation is not successful and
return the error code -ENOMEM.

Signed-off-by Georgiana Rodica Chelu <[email protected]>

---

Changes in v2:
- use a goto pattern to handle the allocation fails
- use exiting a function to free the memory in case of fail
- move the free function above the initialization

 drivers/staging/rtl8192u/r8192U_core.c | 109 ++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 49 deletions(-)
drivers/staging/rtl8192u/r8192U_core.c +60 -49
index b631990..08d7e70 100644
@@ -1677,6 +1677,56 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
 	return -1;
 }
 
+#ifdef THOMAS_BEACON
+static void rtl8192_usb_deleteendpoints(struct net_device *dev)
+{
+	int i;
+	struct r8192_priv *priv = ieee80211_priv(dev);
+
+	if (priv->rx_urb) {
+		for (i = 0; i < (MAX_RX_URB + 1); i++) {
+			usb_kill_urb(priv->rx_urb[i]);
+			usb_free_urb(priv->rx_urb[i]);
+		}
+		kfree(priv->rx_urb);
+		priv->rx_urb = NULL;
+	}
+	kfree(priv->oldaddr);
+	priv->oldaddr = NULL;
+
+	kfree(priv->pp_rxskb);
+	priv->pp_rxskb = NULL;
+}
+#else
+void rtl8192_usb_deleteendpoints(struct net_device *dev)
+{
+	int i;
+	struct r8192_priv *priv = ieee80211_priv(dev);
+
+#ifndef JACKSON_NEW_RX
+
+	if (priv->rx_urb) {
+		for (i = 0; i < (MAX_RX_URB + 1); i++) {
+			usb_kill_urb(priv->rx_urb[i]);
+			kfree(priv->rx_urb[i]->transfer_buffer);
+			usb_free_urb(priv->rx_urb[i]);
+		}
+		kfree(priv->rx_urb);
+		priv->rx_urb = NULL;
+	}
+#else
+	kfree(priv->rx_urb);
+	priv->rx_urb = NULL;
+	kfree(priv->oldaddr);
+	priv->oldaddr = NULL;
+
+	kfree(priv->pp_rxskb);
+	priv->pp_rxskb = 0;
+
+#endif
+}
+#endif
+
 static short rtl8192_usb_initendpoints(struct net_device *dev)
 {
 	struct r8192_priv *priv = ieee80211_priv(dev);

@@ -1689,9 +1739,13 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 #ifndef JACKSON_NEW_RX
 	for (i = 0; i < (MAX_RX_URB + 1); i++) {
 		priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
+		if(!priv->rx_urb[i])
+			goto exit;
 
 		priv->rx_urb[i]->transfer_buffer =
 			kmalloc(RX_URB_SIZE, GFP_KERNEL);
+		if(!priv->rx_urb[i]->transfer_buffer)
+			goto exit;
 
 		priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
 	}

@@ -1704,6 +1758,9 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 
 		priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
 		priv->oldaddr = kmalloc(16, GFP_KERNEL);
+		if(!priv->oldaddr)
+			goto exit;
+
 		oldaddr = priv->oldaddr;
 		align = ((long)oldaddr) & 3;
 		if (align) {

@@ -1732,57 +1789,11 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 
 	netdev_dbg(dev, "End of initendpoints\n");
 	return 0;
-}
-
-#ifdef THOMAS_BEACON
-static void rtl8192_usb_deleteendpoints(struct net_device *dev)
-{
-	int i;
-	struct r8192_priv *priv = ieee80211_priv(dev);
-
-	if (priv->rx_urb) {
-		for (i = 0; i < (MAX_RX_URB + 1); i++) {
-			usb_kill_urb(priv->rx_urb[i]);
-			usb_free_urb(priv->rx_urb[i]);
-		}
-		kfree(priv->rx_urb);
-		priv->rx_urb = NULL;
-	}
-	kfree(priv->oldaddr);
-	priv->oldaddr = NULL;
-
-	kfree(priv->pp_rxskb);
-	priv->pp_rxskb = NULL;
-}
-#else
-void rtl8192_usb_deleteendpoints(struct net_device *dev)
-{
-	int i;
-	struct r8192_priv *priv = ieee80211_priv(dev);
-
-#ifndef JACKSON_NEW_RX
 
-	if (priv->rx_urb) {
-		for (i = 0; i < (MAX_RX_URB + 1); i++) {
-			usb_kill_urb(priv->rx_urb[i]);
-			kfree(priv->rx_urb[i]->transfer_buffer);
-			usb_free_urb(priv->rx_urb[i]);
-		}
-		kfree(priv->rx_urb);
-		priv->rx_urb = NULL;
-	}
-#else
-	kfree(priv->rx_urb);
-	priv->rx_urb = NULL;
-	kfree(priv->oldaddr);
-	priv->oldaddr = NULL;
-
-	kfree(priv->pp_rxskb);
-	priv->pp_rxskb = 0;
-
-#endif
+exit:
+	rtl8192_usb_deleteendpoints(dev);
+	return -ENOMEM;
 }
-#endif
 
 static void rtl8192_update_ratr_table(struct net_device *dev);
 static void rtl8192_link_change(struct net_device *dev)
Signature · 1 line
2.7.4
#1 lore raw
Quoted 2 lines · Georgiana Rodica Chelu

Check if the allocation is not successful and
return the error code -ENOMEM.

I see two issues.

First, in the JACKSON_NEW_RX code you are calling the freeing functions on an array that may not be fully initialized. kfree and usb_free_urb will just return immediately if a NULL pointer is provided as an argument, so things are partially OK. But actually priv->rx_urb[i] will not be NULL in the uninitialized case, because kmalloc was used. Kmalloc allocatesmemory but doesn't zero it. For that you would need kzalloc.

Then in the JACKSON_NEW_RX ifdef of rtl8192_usb_deleteendpoints, it could be that priv->rx_urb[i] is NULL for some values of i, which will give a NULL pointer dereference in priv->rx_urb[i]->transfer_buffer. You may as well test for a NULL value and break out of the loop. But be careful, because it could be that the usb_alloc_urb succeeded and the subsequent kmalloc to initialize priv->rx_urb[i]->transfer_buffer failed.

Finally, at the end of the init function, the error handling code that is present there is not sufficient if either of the previous ifdefs is satisfied.

julia

Quoted 168 lines
Signed-off-by Georgiana Rodica Chelu <[email protected]>

---

Changes in v2:
- use a goto pattern to handle the allocation fails
- use exiting a function to free the memory in case of fail
- move the free function above the initialization

 drivers/staging/rtl8192u/r8192U_core.c | 109 ++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 49 deletions(-)
drivers/staging/rtl8192u/r8192U_core.c +60 -50
index b631990..08d7e70 100644
@@ -1677,6 +1677,56 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
 	return -1;
 }
 
+#ifdef THOMAS_BEACON
+static void rtl8192_usb_deleteendpoints(struct net_device *dev)
+{
+	int i;
+	struct r8192_priv *priv = ieee80211_priv(dev);
+
+	if (priv->rx_urb) {
+		for (i = 0; i < (MAX_RX_URB + 1); i++) {
+			usb_kill_urb(priv->rx_urb[i]);
+			usb_free_urb(priv->rx_urb[i]);
+		}
+		kfree(priv->rx_urb);
+		priv->rx_urb = NULL;
+	}
+	kfree(priv->oldaddr);
+	priv->oldaddr = NULL;
+
+	kfree(priv->pp_rxskb);
+	priv->pp_rxskb = NULL;
+}
+#else
+void rtl8192_usb_deleteendpoints(struct net_device *dev)
+{
+	int i;
+	struct r8192_priv *priv = ieee80211_priv(dev);
+
+#ifndef JACKSON_NEW_RX
+
+	if (priv->rx_urb) {
+		for (i = 0; i < (MAX_RX_URB + 1); i++) {
+			usb_kill_urb(priv->rx_urb[i]);
+			kfree(priv->rx_urb[i]->transfer_buffer);
+			usb_free_urb(priv->rx_urb[i]);
+		}
+		kfree(priv->rx_urb);
+		priv->rx_urb = NULL;
+	}
+#else
+	kfree(priv->rx_urb);
+	priv->rx_urb = NULL;
+	kfree(priv->oldaddr);
+	priv->oldaddr = NULL;
+
+	kfree(priv->pp_rxskb);
+	priv->pp_rxskb = 0;
+
+#endif
+}
+#endif
+
 static short rtl8192_usb_initendpoints(struct net_device *dev)
 {
 	struct r8192_priv *priv = ieee80211_priv(dev);

@@ -1689,9 +1739,13 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 #ifndef JACKSON_NEW_RX
 	for (i = 0; i < (MAX_RX_URB + 1); i++) {
 		priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
+		if(!priv->rx_urb[i])
+			goto exit;
 
 		priv->rx_urb[i]->transfer_buffer =
 			kmalloc(RX_URB_SIZE, GFP_KERNEL);
+		if(!priv->rx_urb[i]->transfer_buffer)
+			goto exit;
 
 		priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
 	}

@@ -1704,6 +1758,9 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 
 		priv->rx_urb[16] = usb_alloc_urb(0, GFP_KERNEL);
 		priv->oldaddr = kmalloc(16, GFP_KERNEL);
+		if(!priv->oldaddr)
+			goto exit;
+
 		oldaddr = priv->oldaddr;
 		align = ((long)oldaddr) & 3;
 		if (align) {

@@ -1732,57 +1789,11 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
 
 	netdev_dbg(dev, "End of initendpoints\n");
 	return 0;
-}
-
-#ifdef THOMAS_BEACON
-static void rtl8192_usb_deleteendpoints(struct net_device *dev)
-{
-	int i;
-	struct r8192_priv *priv = ieee80211_priv(dev);
-
-	if (priv->rx_urb) {
-		for (i = 0; i < (MAX_RX_URB + 1); i++) {
-			usb_kill_urb(priv->rx_urb[i]);
-			usb_free_urb(priv->rx_urb[i]);
-		}
-		kfree(priv->rx_urb);
-		priv->rx_urb = NULL;
-	}
-	kfree(priv->oldaddr);
-	priv->oldaddr = NULL;
-
-	kfree(priv->pp_rxskb);
-	priv->pp_rxskb = NULL;
-}
-#else
-void rtl8192_usb_deleteendpoints(struct net_device *dev)
-{
-	int i;
-	struct r8192_priv *priv = ieee80211_priv(dev);
-
-#ifndef JACKSON_NEW_RX
 
-	if (priv->rx_urb) {
-		for (i = 0; i < (MAX_RX_URB + 1); i++) {
-			usb_kill_urb(priv->rx_urb[i]);
-			kfree(priv->rx_urb[i]->transfer_buffer);
-			usb_free_urb(priv->rx_urb[i]);
-		}
-		kfree(priv->rx_urb);
-		priv->rx_urb = NULL;
-	}
-#else
-	kfree(priv->rx_urb);
-	priv->rx_urb = NULL;
-	kfree(priv->oldaddr);
-	priv->oldaddr = NULL;
-
-	kfree(priv->pp_rxskb);
-	priv->pp_rxskb = 0;
-
-#endif
+exit:
+	rtl8192_usb_deleteendpoints(dev);
+	return -ENOMEM;
 }
-#endif
 
 static void rtl8192_update_ratr_table(struct net_device *dev);
 static void rtl8192_link_change(struct net_device *dev)
--

2.7.4

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170301213000.GA14437%40fireworks.
For more options, visit https://groups.google.com/d/optout.