etherboot-developers

[Etherboot-developers] [PATCH 0/7] IPv4 fragmentation fixes

[Michal K. <mku...@su...>]

These are fixes for some IPv4 fragmentation problems I've found when
investigating problems of one of our customers with PXE boot of a Xen
guest in presence of fragmented packets.

Patches 1-3 are critical, 1 and 2 leading to memory corruption, 3 to
wrong parsing of IPv4 header. Patches 4-6 resolve various memory leaks.
Patch 7 is optional, it limits memory consumption under circumstances
that shouldn't happen but are known to happen from time to time.

Michal Kubecek (7):
  [ipv4] Check for buffer overflow when adding fragment
  [ipv4] Unlink frag_buffer from lists when before freeing
  [ipv4] Convert fragmentation header fields from NBO to HBO
  [ipv4] When freeing frag_buffer, free its io_buffer
  [ipv4] Free packet buffer when fragment isn't matched
  [ipv4] Free frag_buffer when its timer expires
  [ipv4] Limit number of fragment buffers

 src/include/gpxe/ip.h |    4 ++
 src/net/ipv4.c        |   86 ++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 82 insertions(+), 8 deletions(-)

-- 
1.7.7

[Etherboot-developers] [PATCH 3/7] [ipv4] Convert fragmentation header fields from NBO to HBO

[Michal K. <mku...@su...>]

Fragmentation fields of IPv4 header need to be converted from network
byte ordering to host byte ordering to be compatible with IP_MASK_*
constants.

Signed-off-by: Michal Kubecek <mku...@su...>
---
 src/net/ipv4.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/net/ipv4.c b/src/net/ipv4.c
index bd41978..a63738f 100644
--- a/src/net/ipv4.c
+++ b/src/net/ipv4.c
@@ -166,6 +166,7 @@ static void free_fragbuf ( struct frag_buffer *fragbuf ) {
  */
 static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 	struct iphdr *iphdr = iobuf->data;
+	uint16_t pkt_frags = ntohs ( iphdr->frags );
 	struct frag_buffer *fragbuf;
 	struct frag_buffer *fragtmp;
 	
@@ -183,7 +184,7 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 			 * the reassembled I/O buffer
 			 */
 			if ( iob_len ( fragbuf->frag_iob ) == 
-			      ( iphdr->frags & IP_MASK_OFFSET ) ) {
+			      ( pkt_frags & IP_MASK_OFFSET ) ) {
 				/**
 				 * Append the contents of the fragment to the
 				 * reassembled I/O buffer
@@ -203,7 +204,7 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 				free_iob ( iobuf );
 
 				/** Check if the fragment series is over */
-				if ( ! ( iphdr->frags & IP_MASK_MOREFRAGS ) ) {
+				if ( ! ( pkt_frags & IP_MASK_MOREFRAGS ) ) {
 					iobuf = fragbuf->frag_iob;
 					free_fragbuf ( fragbuf );
 					return iobuf;
@@ -219,8 +220,8 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 	}
 	
 	/** Check if the fragment is the first in the fragment series */
-	if ( iphdr->frags & IP_MASK_MOREFRAGS &&
-			( ( iphdr->frags & IP_MASK_OFFSET ) == 0 ) ) {
+	if ( ( pkt_frags & IP_MASK_MOREFRAGS ) &&
+			( ( pkt_frags & IP_MASK_OFFSET ) == 0 ) ) {
 	
 		/** Create a new fragment buffer */
 		fragbuf = ( struct frag_buffer* ) malloc ( sizeof( *fragbuf ) );
-- 
1.7.7

[Etherboot-developers] [PATCH 5/7] [ipv4] Free packet buffer when fragment isn't matched

[Michal K. <mku...@su...>]

When fragment doesn't match any frag_buffer and it isn't a first fragment,
free its io_buffer structure.

Signed-off-by: Michal Kubecek <mku...@su...>
---
 src/net/ipv4.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/net/ipv4.c b/src/net/ipv4.c
index 6c38d70..d698318 100644
--- a/src/net/ipv4.c
+++ b/src/net/ipv4.c
@@ -244,6 +244,9 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 
 		/* Add the fragment buffer to the list of fragment buffers */
 		list_add ( &fragbuf->list, &frag_buffers );
+	} else {
+		/* Doesn't match anything but it's not first fragment */
+		free_iob ( iobuf );
 	}
 	
 	return NULL;
-- 
1.7.7

[Etherboot-developers] [PATCH 7/7] [ipv4] Limit number of fragment buffers

[Michal K. <mku...@su...>]

For every (first) IPv4 fragment received, a buffer of size
IPV4_FRAG_IOB_SIZE (currently 1500 bytes) is allocated. Limiting the
number of fragment buffers prevents memory exhaustion if too many
fragments are received without being assembled (e.g. because of wrongly
configured firewall).

Signed-off-by: Michal Kubecek <mku...@su...>
---
 src/include/gpxe/ip.h |    1 +
 src/net/ipv4.c        |   30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/src/include/gpxe/ip.h b/src/include/gpxe/ip.h
index 29def10..55854ca 100644
--- a/src/include/gpxe/ip.h
+++ b/src/include/gpxe/ip.h
@@ -34,6 +34,7 @@ struct net_protocol;
 
 #define IP_FRAG_IOB_SIZE	1500
 #define IP_FRAG_TIMEOUT		50
+#define IP_FRAG_MAXBUFS		128
 #define IP_FRAG_EXPIRED		0x0001
 
 /** An IPv4 packet header */
diff --git a/src/net/ipv4.c b/src/net/ipv4.c
index f20762f..53048b8 100644
--- a/src/net/ipv4.c
+++ b/src/net/ipv4.c
@@ -34,6 +34,9 @@ struct list_head ipv4_miniroutes = LIST_HEAD_INIT ( ipv4_miniroutes );
 /** List of fragment reassembly buffers */
 static LIST_HEAD ( frag_buffers );
 
+/** Current number of fragment reassembly buffers */
+static int frag_buffer_count = 0;
+
 /**
  * Add IPv4 minirouting table entry
  *
@@ -157,6 +160,7 @@ static void ipv4_frag_expired ( struct retry_timer *timer,
  */
 static void free_fragbuf ( struct frag_buffer *fragbuf ) {
 	list_del ( &fragbuf->list );
+	frag_buffer_count--;
 	stop_timer ( &fragbuf->frag_timer );
 	if ( fragbuf->frag_iob )
 		free_iob ( fragbuf->frag_iob );
@@ -179,6 +183,27 @@ static void ipv4_frag_cleanup ()
 }
 
 /**
+ * Remove oldest fragment buffer
+ *
+ */
+static void ipv4_frag_remove_oldest ()
+{
+	struct frag_buffer *fragbuf;
+	struct frag_buffer *oldest = NULL;
+	unsigned long oldest_start = 0;
+
+	list_for_each_entry ( fragbuf, &frag_buffers, list ) {
+		if ( !oldest || (fragbuf->frag_timer.start < oldest_start) ) {
+			oldest = fragbuf;
+			oldest_start = fragbuf->frag_timer.start;
+		}
+	}
+
+	if ( oldest )
+		free_fragbuf ( oldest );
+}
+
+/**
  * Fragment reassembler
  *
  * @v iobuf		I/O buffer, fragment of the datagram
@@ -246,6 +271,10 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 	/** Check if the fragment is the first in the fragment series */
 	if ( ( pkt_frags & IP_MASK_MOREFRAGS ) &&
 			( ( pkt_frags & IP_MASK_OFFSET ) == 0 ) ) {
+
+		/* If there are too many buffers, remove the oldest */
+		if ( frag_buffer_count >= IP_FRAG_MAXBUFS )
+			ipv4_frag_remove_oldest();
 	
 		/** Create a new fragment buffer */
 		fragbuf = ( struct frag_buffer* ) malloc ( sizeof( *fragbuf ) );
@@ -266,6 +295,7 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 
 		/* Add the fragment buffer to the list of fragment buffers */
 		list_add ( &fragbuf->list, &frag_buffers );
+		frag_buffer_count++;
 	} else {
 		/* Doesn't match anything but it's not first fragment */
 		free_iob ( iobuf );
-- 
1.7.7

[Etherboot-developers] [PATCH 1/7] [ipv4] Check for buffer overflow when adding fragment

[Michal K. <mku...@su...>]

Check whether there is space enough for new fragment to prevent buffer
overflow if fragmented packet larger than IPV4_FRAG_IOB_SIZE (1500 bytes)
is received.

Signed-off-by: Michal Kubecek <mku...@su...>
---
 src/net/ipv4.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/net/ipv4.c b/src/net/ipv4.c
index 92d0684..30c1541 100644
--- a/src/net/ipv4.c
+++ b/src/net/ipv4.c
@@ -186,6 +186,14 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 				 * reassembled I/O buffer
 				 */
 				iob_pull ( iobuf, sizeof ( *iphdr ) );
+				if ( iob_tailroom ( fragbuf->frag_iob ) <
+					iob_len ( iobuf ) )
+				{
+					/* Fragmented packet is too long */
+					free_fragbuf ( fragbuf );
+					free_iob ( iobuf );
+					return NULL;
+				}
 				memcpy ( iob_put ( fragbuf->frag_iob,
 							iob_len ( iobuf ) ),
 					 iobuf->data, iob_len ( iobuf ) );
-- 
1.7.7

[Etherboot-developers] [PATCH 6/7] [ipv4] Free frag_buffer when its timer expires

[Michal K. <mku...@su...>]

In the frag_buffer timer expiry function, there was comment "Free the
fragment buffer" but no attempt to free it was actually done.

Signed-off-by: Michal Kubecek <mku...@su...>
---
 src/include/gpxe/ip.h |    3 +++
 src/net/ipv4.c        |   26 ++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/src/include/gpxe/ip.h b/src/include/gpxe/ip.h
index 4342a0c..29def10 100644
--- a/src/include/gpxe/ip.h
+++ b/src/include/gpxe/ip.h
@@ -34,6 +34,7 @@ struct net_protocol;
 
 #define IP_FRAG_IOB_SIZE	1500
 #define IP_FRAG_TIMEOUT		50
+#define IP_FRAG_EXPIRED		0x0001
 
 /** An IPv4 packet header */
 struct iphdr {
@@ -86,6 +87,8 @@ struct frag_buffer {
 	struct io_buffer *frag_iob;
 	/* Reassembly timer */
 	struct retry_timer frag_timer;
+	/* Flags */
+	uint16_t frag_flags;
 	/* List of fragment reassembly buffers */
 	struct list_head list;
 };
diff --git a/src/net/ipv4.c b/src/net/ipv4.c
index d698318..f20762f 100644
--- a/src/net/ipv4.c
+++ b/src/net/ipv4.c
@@ -139,11 +139,14 @@ static struct ipv4_miniroute * ipv4_route ( struct in_addr *dest ) {
  * @v timer	Retry timer
  * @v over	If asserted, the timer is greater than @c MAX_TIMEOUT 
  */
-static void ipv4_frag_expired ( struct retry_timer *timer __unused,
+static void ipv4_frag_expired ( struct retry_timer *timer,
 				int over ) {
 	if ( over ) {
+		struct frag_buffer *fragbuf;
 		DBG ( "Fragment reassembly timeout" );
-		/* Free the fragment buffer */
+		/* Mark the fragment buffer for cleanup */
+		fragbuf = container_of(timer, struct frag_buffer, frag_timer);
+		fragbuf->frag_flags |= IP_FRAG_EXPIRED;
 	}
 }
 
@@ -161,6 +164,21 @@ static void free_fragbuf ( struct frag_buffer *fragbuf ) {
 }
 
 /**
+ * Remove all expired fragment buffers
+ *
+ */
+static void ipv4_frag_cleanup ()
+{
+	struct frag_buffer *fragbuf;
+	struct frag_buffer *fragtmp;
+
+	list_for_each_entry_safe ( fragbuf, fragtmp, &frag_buffers, list ) {
+		if ( fragbuf->frag_flags & IP_FRAG_EXPIRED )
+			free_fragbuf ( fragbuf );
+	}
+}
+
+/**
  * Fragment reassembler
  *
  * @v iobuf		I/O buffer, fragment of the datagram
@@ -172,6 +190,9 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 	struct frag_buffer *fragbuf;
 	struct frag_buffer *fragtmp;
 	
+	/* Remove expired fragment buffers */
+	ipv4_frag_cleanup();
+
 	/**
 	 * Check if the fragment belongs to any fragment series
 	 */
@@ -228,6 +249,7 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 	
 		/** Create a new fragment buffer */
 		fragbuf = ( struct frag_buffer* ) malloc ( sizeof( *fragbuf ) );
+		fragbuf->frag_flags = 0;
 		fragbuf->ident = iphdr->ident;
 		fragbuf->src = iphdr->src;
 
-- 
1.7.7

[Etherboot-developers] [PATCH 4/7] [ipv4] When freeing frag_buffer, free its io_buffer

[Michal K. <mku...@su...>]

When frag_buffer is freed, we should free its io_buffer structure as well.
However, we don't want to free it when the io_buffer was passed as return
value of ipv4_reassemble().

Signed-off-by: Michal Kubecek <mku...@su...>
---
 src/net/ipv4.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/net/ipv4.c b/src/net/ipv4.c
index a63738f..6c38d70 100644
--- a/src/net/ipv4.c
+++ b/src/net/ipv4.c
@@ -155,6 +155,8 @@ static void ipv4_frag_expired ( struct retry_timer *timer __unused,
 static void free_fragbuf ( struct frag_buffer *fragbuf ) {
 	list_del ( &fragbuf->list );
 	stop_timer ( &fragbuf->frag_timer );
+	if ( fragbuf->frag_iob )
+		free_iob ( fragbuf->frag_iob );
 	free ( fragbuf );
 }
 
@@ -206,6 +208,7 @@ static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 				/** Check if the fragment series is over */
 				if ( ! ( pkt_frags & IP_MASK_MOREFRAGS ) ) {
 					iobuf = fragbuf->frag_iob;
+					fragbuf->frag_iob = NULL;
 					free_fragbuf ( fragbuf );
 					return iobuf;
 				}
-- 
1.7.7

[Etherboot-developers] [PATCH 2/7] [ipv4] Unlink frag_buffer from lists when before freeing

[Michal K. <mku...@su...>]

Freeing frag_buffer structure without unlinking it from frag_buffers
linked list may lead to the list being broken when the memory is reused.
The same problem can happen with timers list if frag_buffer is freed
while its embedded retry_timer is still running.

After this change, list_for_each_entry_safe() has to be used in
ipv4_reassemble() as free_fragbuf() is called inside the cycle body.

Signed-off-by: Michal Kubecek <mku...@su...>
---
 src/net/ipv4.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/net/ipv4.c b/src/net/ipv4.c
index 30c1541..bd41978 100644
--- a/src/net/ipv4.c
+++ b/src/net/ipv4.c
@@ -148,11 +148,13 @@ static void ipv4_frag_expired ( struct retry_timer *timer __unused,
 }
 
 /**
- * Free fragment buffer
+ * Unlink and free fragment buffer
  *
  * @v fragbug	Fragment buffer
  */
 static void free_fragbuf ( struct frag_buffer *fragbuf ) {
+	list_del ( &fragbuf->list );
+	stop_timer ( &fragbuf->frag_timer );
 	free ( fragbuf );
 }
 
@@ -165,11 +167,12 @@ static void free_fragbuf ( struct frag_buffer *fragbuf ) {
 static struct io_buffer * ipv4_reassemble ( struct io_buffer * iobuf ) {
 	struct iphdr *iphdr = iobuf->data;
 	struct frag_buffer *fragbuf;
+	struct frag_buffer *fragtmp;
 	
 	/**
 	 * Check if the fragment belongs to any fragment series
 	 */
-	list_for_each_entry ( fragbuf, &frag_buffers, list ) {
+	list_for_each_entry_safe ( fragbuf, fragtmp, &frag_buffers, list ) {
 		if ( fragbuf->ident == iphdr->ident &&
 		     fragbuf->src.s_addr == iphdr->src.s_addr ) {
 			/**
-- 
1.7.7