Patchwork arp: fix possible crash in arp_rcv()

login
register
mail settings
Submitter Eric Dumazet
Date Feb. 9, 2013, 4:48 a.m.
Message ID <1360385301.6696.11.camel@edumazet-glaptop>
Download mbox | patch
Permalink /patch/219367/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Feb. 9, 2013, 4:48 a.m.
From: Eric Dumazet <edumazet@google.com>

We should call skb_share_check() before pskb_may_pull(), or we
can crash in pskb_expand_head()

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/arp.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 11, 2013, 1:40 a.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 08 Feb 2013 20:48:21 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> We should call skb_share_check() before pskb_may_pull(), or we
> can crash in pskb_expand_head()
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

It's just a very odd set of combined circumstances we would
need for this to trigger and also cause a real problem of
any kind.

In any event, the fix is of course correct, and thus applied
to 'net'.  Thanks Eric!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 9547a273..ded146b 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -928,24 +928,25 @@  static void parp_redo(struct sk_buff *skb)
 static int arp_rcv(struct sk_buff *skb, struct net_device *dev,
 		   struct packet_type *pt, struct net_device *orig_dev)
 {
-	struct arphdr *arp;
+	const struct arphdr *arp;
+
+	if (dev->flags & IFF_NOARP ||
+	    skb->pkt_type == PACKET_OTHERHOST ||
+	    skb->pkt_type == PACKET_LOOPBACK)
+		goto freeskb;
+
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (!skb)
+		goto out_of_mem;
 
 	/* ARP header, plus 2 device addresses, plus 2 IP addresses.  */
 	if (!pskb_may_pull(skb, arp_hdr_len(dev)))
 		goto freeskb;
 
 	arp = arp_hdr(skb);
-	if (arp->ar_hln != dev->addr_len ||
-	    dev->flags & IFF_NOARP ||
-	    skb->pkt_type == PACKET_OTHERHOST ||
-	    skb->pkt_type == PACKET_LOOPBACK ||
-	    arp->ar_pln != 4)
+	if (arp->ar_hln != dev->addr_len || arp->ar_pln != 4)
 		goto freeskb;
 
-	skb = skb_share_check(skb, GFP_ATOMIC);
-	if (skb == NULL)
-		goto out_of_mem;
-
 	memset(NEIGH_CB(skb), 0, sizeof(struct neighbour_cb));
 
 	return NF_HOOK(NFPROTO_ARP, NF_ARP_IN, skb, dev, NULL, arp_process);