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

[Marcelo Tosatti]

Hi Holger,

That line sneaked somehow with the patch - it was part of the original
driver.

process_rxed_802_11_packet is never called at the moment, but we might
want to support monitor mode properly (there is a firmware comming out
with it soon), and setting ETH_P_80211_RAW seems required for that.

So for now I would do this instead

diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
index a84e100..1ec54e1 100644
--- a/drivers/net/wireless/libertas/rx.c
+++ b/drivers/net/wireless/libertas/rx.c
@@ -432,13 +432,13 @@ static int process_rxed_802_11_packet(wl
 	lbs_deb_rx("rx data: size of actual packet %d\n", skb->len);
 	priv->stats.rx_bytes += skb->len;
 	priv->stats.rx_packets++;
+	skb->protocol = __constant_htons(0x0019);	/* ETH_P_80211_RAW */
 
 	libertas_upload_rx_packet(priv, skb);
 
 	ret = 0;
 
 done:
-	skb->protocol = __constant_htons(0x0019);	/* ETH_P_80211_RAW */
 	lbs_deb_leave_args(LBS_DEB_RX, "ret %d", ret);
 	return ret;
 }



On Sat, Jun 30, 2007 at 09:14:53PM +0200, Holger Schurig wrote:
> Hi Marcello,
> 
> see the attached message from Adrian bunk.
> 
> I've done a
> 
>    git-show 9012b28a407511fb355f6d2176a12d4653489672
> 
> and indeed I'm seeing the line
> 
>    skb->protocol = __constant_htons(0x0019);       /* ETH_P_80211_RAW */
> 
> You did the commit into the libertas-git tree for me.
> 
> However, I don't have the slightest clue how this line
> made it into this patch. I suggested this patch in on
> 23rd February with the subject "patch to taylor debug
> output", but the link to the git tree where the patch
> was in is gone.
> 
> However, the above git-show did show a date of May, 25th.
> How comes?  My patch was from February?!?!
> 
> And I'm quite sure that I didn't do anything in my
> patch with skb->protocol. I hardly know anything about
> skb's, just learnt recently for my if_cs.c file, so I
> cannot imagine that I ever set any protocol field while
> not knowing what's going on there. Did this line slip
> in from your own code?
> 
> In rx.c before commit 9012b28a407511fb355f6d2176a12d4653489672,
> there is no mentioning of skb->protocol at all. Do you
> think the right fix is to simply remove this line?

Content-Description: Adrian Bunk <bunk at stusta.de>: drivers/net/wireless/libertas/rx.c: use-after-free
> From: Adrian Bunk <bunk at stusta.de>
> Date: Fri, 29 Jun 2007 21:51:16 +0200
> To: linville at tuxdriver.com,
>  Holger Schurig <hs4233 at mail.mn-solutions.de>
> Cc: linux-wireless at vger.kernel.org,
>  linux-kernel at vger.kernel.org
> Subject: drivers/net/wireless/libertas/rx.c: use-after-free
> X-Length: 2861
> 
> The Coverity checker spotted the following use-after-free of "skb" in 
> drivers/net/wireless/libertas/rx.c introduced by
> commit 9012b28a407511fb355f6d2176a12d4653489672 (WTF did this commit
> with the title "libertas: make debug configurable" add the 
> "skb->protocol = __constant_htons(0x0019);" line?):
> 
> <--  snip  -->
> 
> ...
> static int process_rxed_802_11_packet(wlan_private * priv, struct sk_buff *skb)
> {
> ...
>         libertas_upload_rx_packet(priv, skb);
> 
>         ret = 0;
> 
> done:
>         skb->protocol = __constant_htons(0x0019);       /* ETH_P_80211_RAW */
> ...
> 
> <--  snip  -->
> 
> 
> cu
> Adrian
> 
> -- 
> 
>        "Is there not promise of rain?" Ling Tan asked suddenly out
>         of the darkness. There had been need of rain for many days.
>        "Only a promise," Lao Er said.
>                                        Pearl S. Buck - Dragon Seed
> 
> 
>