Browse Source

[arp] Prevent ARP cache entries from being deleted mid-transmission

Each ARP cache entry maintains a transmission queue, which is sent out
as soon as the link-layer address is known.  If multiple packets are
queued, then it is possible for memory pressure to cause the ARP cache
discarder to be invoked during transmission of the first packet, which
may cause the ARP cache entry to be deleted before the second packet
can be sent.  This results in an invalid pointer dereference.

Avoid this problem by reference-counting ARP cache entries and
ensuring that an extra reference is held while processing the
transmission queue, and by using list_first_entry() rather than
list_for_each_entry_safe() to traverse the queue.

Signed-off-by: Michael Brown <mcb30@ipxe.org>
tags/v1.20.1
Michael Brown 12 years ago
parent
commit
19859d8ead
1 changed files with 38 additions and 11 deletions
  1. 38
    11
      src/net/arp.c

+ 38
- 11
src/net/arp.c View File

@@ -31,6 +31,7 @@ FILE_LICENCE ( GPL2_OR_LATER );
31 31
 #include <ipxe/retry.h>
32 32
 #include <ipxe/timer.h>
33 33
 #include <ipxe/malloc.h>
34
+#include <ipxe/refcnt.h>
34 35
 #include <ipxe/arp.h>
35 36
 
36 37
 /** @file
@@ -51,6 +52,8 @@ FILE_LICENCE ( GPL2_OR_LATER );
51 52
 
52 53
 /** An ARP cache entry */
53 54
 struct arp_entry {
55
+	/** Reference count */
56
+	struct refcnt refcnt;
54 57
 	/** List of ARP cache entries */
55 58
 	struct list_head list;
56 59
 	/** Network device */
@@ -76,6 +79,25 @@ struct net_protocol arp_protocol __net_protocol;
76 79
 
77 80
 static void arp_expired ( struct retry_timer *timer, int over );
78 81
 
82
+/**
83
+ * Free ARP cache entry
84
+ *
85
+ * @v refcnt		Reference count
86
+ */
87
+static void arp_free ( struct refcnt *refcnt ) {
88
+	struct arp_entry *arp =
89
+		container_of ( refcnt, struct arp_entry, refcnt );
90
+
91
+	/* Sanity check */
92
+	assert ( list_empty ( &arp->tx_queue ) );
93
+
94
+	/* Drop reference to network device */
95
+	netdev_put ( arp->netdev );
96
+
97
+	/* Free entry */
98
+	free ( arp );
99
+}
100
+
79 101
 /**
80 102
  * Create ARP cache entry
81 103
  *
@@ -91,27 +113,28 @@ static struct arp_entry * arp_create ( struct net_device *netdev,
91 113
 				       const void *net_source ) {
92 114
 	struct arp_entry *arp;
93 115
 
94
-	/* Allocate entry and add to cache */
116
+	/* Allocate and initialise entry */
95 117
 	arp = zalloc ( sizeof ( *arp ) );
96 118
 	if ( ! arp )
97 119
 		return NULL;
98
-
99
-	/* Initialise entry and add to cache */
120
+	ref_init ( &arp->refcnt, arp_free );
100 121
 	arp->netdev = netdev_get ( netdev );
101 122
 	arp->net_protocol = net_protocol;
102 123
 	memcpy ( arp->net_dest, net_dest,
103 124
 		 net_protocol->net_addr_len );
104 125
 	memcpy ( arp->net_source, net_source,
105 126
 		 net_protocol->net_addr_len );
106
-	timer_init ( &arp->timer, arp_expired, NULL );
127
+	timer_init ( &arp->timer, arp_expired, &arp->refcnt );
107 128
 	arp->timer.min_timeout = ARP_MIN_TIMEOUT;
108 129
 	arp->timer.max_timeout = ARP_MAX_TIMEOUT;
109 130
 	INIT_LIST_HEAD ( &arp->tx_queue );
110
-	list_add ( &arp->list, &arp_entries );
111 131
 
112 132
 	/* Start timer running to trigger initial transmission */
113 133
 	start_timer_nodelay ( &arp->timer );
114 134
 
135
+	/* Transfer ownership to cache */
136
+	list_add ( &arp->list, &arp_entries );
137
+
115 138
 	DBGC ( arp, "ARP %p %s %s %s created\n", arp, netdev->name,
116 139
 	       net_protocol->name, net_protocol->ntoa ( net_dest ) );
117 140
 	return arp;
@@ -174,10 +197,9 @@ static void arp_destroy ( struct arp_entry *arp, int rc ) {
174 197
 	       net_protocol->name, net_protocol->ntoa ( arp->net_dest ),
175 198
 	       strerror ( rc ) );
176 199
 
177
-	/* Drop reference to network device, remove from cache and free */
178
-	netdev_put ( arp->netdev );
200
+	/* Remove from cache and drop reference */
179 201
 	list_del ( &arp->list );
180
-	free ( arp );
202
+	ref_put ( &arp->refcnt );
181 203
 }
182 204
 
183 205
 /**
@@ -241,7 +263,6 @@ static void arp_update ( struct arp_entry *arp, const void *ll_dest ) {
241 263
 	struct ll_protocol *ll_protocol = netdev->ll_protocol;
242 264
 	struct net_protocol *net_protocol = arp->net_protocol;
243 265
 	struct io_buffer *iobuf;
244
-	struct io_buffer *tmp;
245 266
 	int rc;
246 267
 
247 268
 	DBGC ( arp, "ARP %p %s %s %s updated => %s\n", arp, netdev->name,
@@ -254,8 +275,13 @@ static void arp_update ( struct arp_entry *arp, const void *ll_dest ) {
254 275
 	/* Stop retransmission timer */
255 276
 	stop_timer ( &arp->timer );
256 277
 
257
-	/* Transmit any packets in queue */
258
-	list_for_each_entry_safe ( iobuf, tmp, &arp->tx_queue, list ) {
278
+	/* Transmit any packets in queue.  Take out a temporary
279
+	 * reference on the entry to prevent it from going out of
280
+	 * scope during the call to net_tx().
281
+	 */
282
+	ref_get ( &arp->refcnt );
283
+	while ( ( iobuf = list_first_entry ( &arp->tx_queue, struct io_buffer,
284
+					     list ) ) != NULL ) {
259 285
 		DBGC2 ( arp, "ARP %p %s %s %s transmitting deferred packet\n",
260 286
 			arp, netdev->name, net_protocol->name,
261 287
 			net_protocol->ntoa ( arp->net_dest ) );
@@ -267,6 +293,7 @@ static void arp_update ( struct arp_entry *arp, const void *ll_dest ) {
267 293
 			/* Ignore error and continue */
268 294
 		}
269 295
 	}
296
+	ref_put ( &arp->refcnt );
270 297
 }
271 298
 
272 299
 /**

Loading…
Cancel
Save