netdev - Re: [PATCH] libertas: skb dereferenced after netif

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <464E06D1.9070804@gmail.com>
Date:	Fri, 18 May 2007 16:04:33 -0400
From:	Florin Malita <fmalita@...il.com>
To:	"John W. Linville" <linville@...driver.com>
CC:	marcelo@...ck.org, linville@...hat.com, netdev@...r.kernel.org,
	linux-wireless@...r.kernel.org
Subject: Re: [PATCH] libertas: skb dereferenced after netif_rx

John W. Linville wrote:
>> Also, libertas_upload_rx_packet() unconditionally returns 0 so the error 
>> check is dead code - might as well take it out.
>>     
>
> Is this merely an implementation detail?  Or an absolute fact?
>   

I believe it's an absolute fact that got lost among implementation 
details ;)

All libertas_upload_rx_packet does is set a few fields in the skb, then 
pass it up to the stack via netif_rx:

139 int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
140 {
141         lbs_pr_debug(1, "skb->data=%p\n", skb->data);
142
143         if(IS_MESH_FRAME(skb))
144                 skb->dev = priv->mesh_dev;
145         else
146                 skb->dev = priv->wlan_dev.netdev;
147         skb->protocol = eth_type_trans(skb, priv->wlan_dev.netdev);
148         skb->ip_summed = CHECKSUM_UNNECESSARY;
149
150         netif_rx(skb);
151
152         return 0;
153 }


Since netif_rx always succeeds, so should libertas_upload_rx_packet - 
there's no reason for passing back a success code (especially one that's 
hardcoded to 0).

> If the latter, then we should change the signature of
> libertas_upload_rx_packet to return void.
>   

Makes sense, updated patch below.

> Another potential patch is to remove the "ret = 0" line before the
> "done" label, since ret is initialized at the head of the function.
> Come to think of it, you can probably remove the "= 0" part of ret's
> declaration as well (in both functions).
>   

Right, even more: looks like both process_rxed_802_11_packet & 
libertas_process_rxed_packet can only return 0 so we could drop the 
return code altogether and change their signature to void too (nobody 
seems to care about their return code anyway). I will send a separate 
cleanup patch but this might be leaning more on the implementation 
detail side (planning to extend the functions and make the return code 
meaningful in the future?) so somebody familiar with the driver should 
make the call.

Thanks,
Florin


Signed-off-by: Florin Malita <fmalita@...il.com>
---

 decl.h |    2 +-
 rx.c   |   22 +++++-----------------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/libertas/decl.h b/drivers/net/wireless/libertas/decl.h
index 606bdd0..dfe2764 100644
--- a/drivers/net/wireless/libertas/decl.h
+++ b/drivers/net/wireless/libertas/decl.h
@@ -46,7 +46,7 @@ u32 libertas_index_to_data_rate(u8 index);
 u8 libertas_data_rate_to_index(u32 rate);
 void libertas_get_fwversion(wlan_adapter * adapter, char *fwversion, int maxlen);
 
-int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb);
+void libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb);
 
 /** The proc fs interface */
 int libertas_process_rx_command(wlan_private * priv);
diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
index d17924f..b19b5aa 100644
--- a/drivers/net/wireless/libertas/rx.c
+++ b/drivers/net/wireless/libertas/rx.c
@@ -136,7 +136,7 @@ static void wlan_compute_rssi(wlan_private * priv, struct rxpd *p_rx_pd)
 	LEAVE();
 }
 
-int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
+void libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
 {
 	lbs_pr_debug(1, "skb->data=%p\n", skb->data);
 
@@ -148,8 +148,6 @@ int libertas_upload_rx_packet(wlan_private * priv, struct sk_buff *skb)
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	netif_rx(skb);
-
-	return 0;
 }
 
 /**
@@ -269,15 +267,11 @@ 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();
@@ -438,17 +432,11 @@ static int process_rxed_802_11_packet(wlan_private * priv, struct sk_buff *skb)
 	wlan_compute_rssi(priv, prxpd);
 
 	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();

-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html