drivers/net/wireless/libertas/rx.c: fix use-after-free

PATCH In review Eugene Teo 3 participants 5 messages May 18, 2007 – May 21, 2007 Networking drivers (wireless)

Share & tools

Eugene Teo

author

2007-05-18 16:52 UTC #0 lore raw

PATCH 1/1 drivers/net/wireless/libertas/rx.c: fix use-after-free

libertas_upload_rx_packet() calls netif_rx() before returning, and it always return 0. Also within libertas_upload_rx_packet(), it will initialize skb->protocol anyways.

Spotted by the Coverity checker.

Signed-off-by Eugene Teo <[email protected]>

drivers/net/wireless/libertas/rx.c +3 -6

index d17924f..1d8d5e4 100644
@@ -269,15 +269,12 @@ int libertas_process_rxed_packet(wlan_private * priv, struct sk_buff *skb)
        wlan_compute_rssi(priv, p_rx_pd);
 
        lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);
-       if (libertas_upload_rx_packet(priv, skb)) {
-               lbs_pr_debug(1, "RX error: libertas_upload_rx_packet"
-                      " returns failure\n");
-               ret = -1;
-               goto done;
-       }
+
        priv->stats.rx_bytes += skb->len;
        priv->stats.rx_packets++;
 
+       libertas_upload_rx_packet(priv, skb);
+
        ret = 0;
 done:
        LEAVE();

@@ -439,21 +436,14 @@ static int process_rxed_802_11_packet(wlan_private * priv, struct

sk_buff *skb)

        lbs_pr_debug(1, "RX Data: size of actual packet = %d\n", skb->len);

- if (libertas_upload_rx_packet(priv, skb)) {
- lbs_pr_debug(1, "RX error: libertas_upload_rx_packet "
- "returns failure\n");
- ret = -1;
- goto done;
- }
-

        priv->stats.rx_bytes += skb->len;
        priv->stats.rx_packets++;

+ libertas_upload_rx_packet(priv, skb);
+

        ret = 0;
 done:
        LEAVE();

- skb->protocol = __constant_htons(0x0019); /* ETH_P_80211_RAW */
-

John W. Linville

2007-05-18 18:02 UTC #1 lore raw

First, please send all wireless patches to [email protected], and be sure to CC me as well...thanks!

Quoted 4 lines · Eugene Teo

libertas_upload_rx_packet() calls netif_rx() before returning, and it always return 0. Also within libertas_upload_rx_packet(), it will initialize skb->protocol anyways.
Spotted by the Coverity checker.

A nearly identical patch was posted by Florin Malita <[email protected]> to netdev (also the wrong list) on Wednesday evening.

Review

Quoted 5 lines

done:

        LEAVE();

(quoted diff) +0 -2

-       skb->protocol = __constant_htons(0x0019);       /* ETH_P_80211_RAW */
-

Except for this part...is this intentional?

John

Signature · 2 lines

John W. Linville
[email protected]

Eugene Teo

author

2007-05-19 03:17 UTC #2 lore raw

Quoted 11 lines · John W. Linville

First, please send all wireless patches to [email protected], and be sure to CC me as well...thanks!
Quoted 4 lines · Eugene Teo
libertas_upload_rx_packet() calls netif_rx() before returning, and it always return 0. Also within libertas_upload_rx_packet(), it will initialize skb->protocol anyways.
Spotted by the Coverity checker.
A nearly identical patch was posted by Florin Malita <[email protected]> to netdev (also the wrong list) on Wednesday evening.

Nod. I wasn't subscribed to netdev list.

Quoted 7 lines

Review
Quoted 5 lines
done:
        LEAVE();
(quoted diff) +0 -2
-       skb->protocol = __constant_htons(0x0019);       /* ETH_P_80211_RAW */
-
Except for this part...is this intentional?

skb could have been freed by then. And, in libertas_upload_rx_packet(), skb->protocol is initialized by eth_type_trans(skb, priv->wlan_dev.netdev).

Eugene

John W. Linville

2007-05-21 14:02 UTC #3 lore raw

Quoted 5 lines · Eugene Teo

Quoted 3 lines · John W. Linville
Quoted 3 lines
done:
        LEAVE();

Quoted 7 lines

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +0 -2
-       skb->protocol = __constant_htons(0x0019);       /* ETH_P_80211_RAW */
-
Except for this part...is this intentional?
skb could have been freed by then. And, in libertas_upload_rx_packet(), skb->protocol is initialized by eth_type_trans(skb, priv->wlan_dev.netdev).

OK, I see that. Looks like Florin has reposted his patch, still without this hunk. Would you like to submit a patch for this hunk?

Thanks,

John

Signature · 2 lines

John W. Linville
[email protected]

Eugene Teo

2007-05-21 14:30 UTC #4 lore raw

Quoted 9 lines · Eugene Teo

Quoted 8 lines
Quoted 6 lines · John W. Linville
Quoted 5 lines
done:
        LEAVE();
(quoted diff) +0 -2
-       skb->protocol = __constant_htons(0x0019);       /* ETH_P_80211_RAW */
-

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Except for this part...is this intentional?
skb could have been freed by then. And, in libertas_upload_rx_packet(), skb->protocol is initialized by eth_type_trans(skb, priv->wlan_dev.netdev).
OK, I see that. Looks like Florin has reposted his patch, still without this hunk. Would you like to submit a patch for this hunk?

skb could have been freed by then. Also, in libertas_upload_rx_packet(), skb->protocol is initialized by eth_type_trans().

Cc John W. Linville <[email protected]>

Signed-off-by Eugene Teo <[email protected]>

---

drivers/net/wireless/libertas/rx.c +0 -2

diff -uprN -X 2.6.22-rc2/Documentation/dontdiff 2.6.22-rc2.default/drivers/net/wireless/libertas/rx.c 2.6.22-rc2/drivers/net/wireless/libertas/rx.c
@@ -453,7 +453,5 @@ static int process_rxed_802_11_packet(wl
 done:
 	LEAVE();
 
-	skb->protocol = __constant_htons(0x0019);	/* ETH_P_80211_RAW */
-
 	return (ret);
 }

Signature · 2 lines

1024D/58DF8823 print 47B9 90F6 AE4A 9C51 37E0  D6E1 EA84 C6A2 58DF 8823
main(i) { putchar(182623909 >> (i-1) * 5&31|!!(i<7)<<6) && main(++i); }