diff mbox

[V2] fwts-alloc: track memory allocations, cleans up 6 coverity scan warnings

Message ID 20170202163908.16017-1-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King Feb. 2, 2017, 4:39 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

CoverityScan was warning about use of tainted pointers on memory
re-allocations and frees as well as mistrusted data read from the
/proc/self/maps.  This fix addresses these issues by keeping a
record of all allocations and sanity checking pointers on re-allocs
and frees so that we can be sure we can trust the pointers to
the memory regions.  The fix also performs some more robust
sanity range checks on address ranges read from /proc/self/maps.

Since fwts only makes 50 or so low memory allocations for cached
ACPI data we don't need a large hash table; 509 slots were used
as a prime sized hash tables work well with power of two sized
data to hash, plus we normally end up with direct hashing into
the table so we get fast lookup.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_alloc.c | 135 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 124 insertions(+), 11 deletions(-)

Comments

Alex Hung Feb. 7, 2017, 3:23 a.m. UTC | #1 
On 2017-02-03 12:39 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> CoverityScan was warning about use of tainted pointers on memory
> re-allocations and frees as well as mistrusted data read from the
> /proc/self/maps.  This fix addresses these issues by keeping a
> record of all allocations and sanity checking pointers on re-allocs
> and frees so that we can be sure we can trust the pointers to
> the memory regions.  The fix also performs some more robust
> sanity range checks on address ranges read from /proc/self/maps.
>
> Since fwts only makes 50 or so low memory allocations for cached
> ACPI data we don't need a large hash table; 509 slots were used
> as a prime sized hash tables work well with power of two sized
> data to hash, plus we normally end up with direct hashing into
> the table so we get fast lookup.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/lib/src/fwts_alloc.c | 135 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 124 insertions(+), 11 deletions(-)
>
> diff --git a/src/lib/src/fwts_alloc.c b/src/lib/src/fwts_alloc.c
> index 09e7c93..1901a53 100644
> --- a/src/lib/src/fwts_alloc.c
> +++ b/src/lib/src/fwts_alloc.c
> @@ -27,6 +27,92 @@
>  #include "fwts.h"
>
>  /*
> + *  Typically we use around 50 or so low mem allocations in fwts
> + *  so 509 is a good large prime size to ensure we get a good
> + *  hash table hit rate
> + */
> +#define HASH_ALLOC_SIZE	(509)
> +
> +typedef struct hash_alloc {
> +	void *addr;		/* allocation addr, NULL = not used */
> +	struct hash_alloc *next;/* next one in hash chain */
> +} hash_alloc_t;
> +
> +static hash_alloc_t *hash_allocs[HASH_ALLOC_SIZE];
> +
> +/*
> + *  hash_addr()
> + *	turn a void * address into a hash index
> + */
> +static inline size_t hash_addr(const void *addr)
> +{
> +	ptrdiff_t h = (ptrdiff_t)addr;
> +
> +	h ^= h >> 17;
> +	h %= HASH_ALLOC_SIZE;
> +
> +	return (size_t)h;
> +}
> +
> +/*
> + *  hash_alloc_add()
> + *	add a allocated address into the hash of
> + *	known allocations so we can keep track
> + *	of valid allocs.
> + */
> +static bool hash_alloc_add(void *addr)
> +{
> +	size_t h = hash_addr(addr);
> +	hash_alloc_t *new = hash_allocs[h];
> +
> +	while (new) {
> +		/* re-use old nullified records */
> +		if (new->addr == NULL) {
> +			/* old and free, so re-use */
> +			new->addr = addr;
> +			return true;
> +		}
> +		/* something is wrong, already in use */
> +		if (new->addr == addr) {
> +			return false;
> +		}
> +		new = new->next;
> +	}
> +
> +	/* not found, add a new record */
> +	new = malloc(sizeof(*new));
> +	if (!new)
> +		return false;
> +
> +	new->addr = addr;
> +	new->next = hash_allocs[h];
> +	hash_allocs[h] = new;
> +
> +	return true;
> +}
> +
> +/*
> + *  hash_alloc_remove()
> + *	remove an allocated address from the hash
> + *	of know allocations.
> + */
> +static bool hash_alloc_remove(const void *addr)
> +{
> +	size_t h = hash_addr(addr);
> +	hash_alloc_t *ha = hash_allocs[h];
> +
> +	while (ha) {
> +		if (ha->addr == addr) {
> +			/* Just nullify it */
> +			ha->addr = NULL;
> +			return true;
> +		}
> +		ha = ha->next;
> +	}
> +	return false;
> +}
> +
> +/*
>   * We implement a low memory allocator to allow us to allocate
>   * memory < 2G limit for the ACPICA table handling.  On 64 bit
>   * machines we have to ensure that cached copies of ACPI tables
> @@ -111,7 +197,6 @@ static void *fwts_low_mmap_walkdown(const size_t requested_size)
>  static void *fwts_low_mmap(const size_t requested_size)
>  {
>  	FILE *fp;
> -	char buffer[1024];
>  	char pathname[1024];
>  	void *addr_start;
>  	void *addr_end;
> @@ -129,15 +214,23 @@ static void *fwts_low_mmap(const size_t requested_size)
>  	if ((fp = fopen("/proc/self/maps", "r")) == NULL)
>  		return fwts_low_mmap_walkdown(requested_size);
>
> -	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
> -		if (sscanf(buffer, "%p-%p %*s %*x %*s %*u %1023s",
> +	while (!feof(fp)) {
> +		if (fscanf(fp, "%p-%p %*s %*x %*s %*u %1023s\n",
>  		    &addr_start, &addr_end, pathname) != 3)
>  			continue;
>  		/*
> +		 *  Sanity check data
> +		 */
> +		if ((addr_start <= (void *)LIMIT_START) ||
> +		    (addr_start >= (void *)LIMIT_2GB) ||
> +		    (addr_end <= (void *)LIMIT_START) ||
> +		    (addr_end >= (void *)LIMIT_2GB) ||
> +		    (addr_end <= addr_start))
> +			continue;
> +		/*
>  		 *  Try and allocate under first mmap'd address space
>  		 */
> -		if ((first_addr_start == NULL) &&
> -		    (addr_start > (void*)LIMIT_START)) {
> +		if (first_addr_start == NULL) {
>  			size_t sz = (requested_size + CHUNK_SIZE) & ~(CHUNK_SIZE - 1);
>  			uint8_t *addr = (uint8_t *)addr_start - sz;
>
> @@ -241,7 +334,14 @@ void *fwts_low_calloc(const size_t nmemb, const size_t size)
>  	hdr->size = n;
>  	hdr->magic = FWTS_ALLOC_MAGIC;
>
> -	return (void *)((uint8_t *)ret + sizeof(fwts_mmap_header));
> +	ret = (void *)((uint8_t *)ret + sizeof(fwts_mmap_header));
> +
> +	if (!hash_alloc_add(ret)) {
> +		munmap((void *)hdr, n);
> +		return NULL;
> +	}
> +
> +	return ret;
>  }
>
>  /*
> @@ -287,10 +387,23 @@ void *fwts_low_realloc(const void *ptr, const size_t size)
>   */
>  void fwts_low_free(const void *ptr)
>  {
> -	if (ptr) {
> -		fwts_mmap_header *hdr = (fwts_mmap_header *)
> -			((uint8_t *)ptr - sizeof(fwts_mmap_header));
> -		if (hdr->magic == FWTS_ALLOC_MAGIC)
> -			munmap(hdr, hdr->size);
> +	/*
> +	 *  Sanity check the address we are about to
> +	 *  try and free. If it is not known about
> +	 *  the don't free it.
> +	 */
> +	if (!ptr)
> +		return;
> +	if (!hash_alloc_remove(ptr)) {
> +		/* Should never happen... */
> +		fprintf(stderr, "double free on %p\n", ptr);
> +		return;
>  	}
> +
> +	fwts_mmap_header *hdr = (fwts_mmap_header *)
> +		((uint8_t *)ptr - sizeof(fwts_mmap_header));
> +
> +	/* Be doubly sure by checking magic before we munmap */
> +	if (hdr->magic == FWTS_ALLOC_MAGIC)
> +		munmap(hdr, hdr->size);
>  }
>

Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu Feb. 8, 2017, 4:30 a.m. UTC | #2 
On 2017年02月03日 00:39, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> CoverityScan was warning about use of tainted pointers on memory
> re-allocations and frees as well as mistrusted data read from the
> /proc/self/maps.  This fix addresses these issues by keeping a
> record of all allocations and sanity checking pointers on re-allocs
> and frees so that we can be sure we can trust the pointers to
> the memory regions.  The fix also performs some more robust
> sanity range checks on address ranges read from /proc/self/maps.
>
> Since fwts only makes 50 or so low memory allocations for cached
> ACPI data we don't need a large hash table; 509 slots were used
> as a prime sized hash tables work well with power of two sized
> data to hash, plus we normally end up with direct hashing into
> the table so we get fast lookup.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/lib/src/fwts_alloc.c | 135 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 124 insertions(+), 11 deletions(-)
>
> diff --git a/src/lib/src/fwts_alloc.c b/src/lib/src/fwts_alloc.c
> index 09e7c93..1901a53 100644
> --- a/src/lib/src/fwts_alloc.c
> +++ b/src/lib/src/fwts_alloc.c
> @@ -27,6 +27,92 @@
>  #include "fwts.h"
>
>  /*
> + *  Typically we use around 50 or so low mem allocations in fwts
> + *  so 509 is a good large prime size to ensure we get a good
> + *  hash table hit rate
> + */
> +#define HASH_ALLOC_SIZE	(509)
> +
> +typedef struct hash_alloc {
> +	void *addr;		/* allocation addr, NULL = not used */
> +	struct hash_alloc *next;/* next one in hash chain */
> +} hash_alloc_t;
> +
> +static hash_alloc_t *hash_allocs[HASH_ALLOC_SIZE];
> +
> +/*
> + *  hash_addr()
> + *	turn a void * address into a hash index
> + */
> +static inline size_t hash_addr(const void *addr)
> +{
> +	ptrdiff_t h = (ptrdiff_t)addr;
> +
> +	h ^= h >> 17;
> +	h %= HASH_ALLOC_SIZE;
> +
> +	return (size_t)h;
> +}
> +
> +/*
> + *  hash_alloc_add()
> + *	add a allocated address into the hash of
> + *	known allocations so we can keep track
> + *	of valid allocs.
> + */
> +static bool hash_alloc_add(void *addr)
> +{
> +	size_t h = hash_addr(addr);
> +	hash_alloc_t *new = hash_allocs[h];
> +
> +	while (new) {
> +		/* re-use old nullified records */
> +		if (new->addr == NULL) {
> +			/* old and free, so re-use */
> +			new->addr = addr;
> +			return true;
> +		}
> +		/* something is wrong, already in use */
> +		if (new->addr == addr) {
> +			return false;
> +		}
> +		new = new->next;
> +	}
> +
> +	/* not found, add a new record */
> +	new = malloc(sizeof(*new));
> +	if (!new)
> +		return false;
> +
> +	new->addr = addr;
> +	new->next = hash_allocs[h];
> +	hash_allocs[h] = new;
> +
> +	return true;
> +}
> +
> +/*
> + *  hash_alloc_remove()
> + *	remove an allocated address from the hash
> + *	of know allocations.
> + */
> +static bool hash_alloc_remove(const void *addr)
> +{
> +	size_t h = hash_addr(addr);
> +	hash_alloc_t *ha = hash_allocs[h];
> +
> +	while (ha) {
> +		if (ha->addr == addr) {
> +			/* Just nullify it */
> +			ha->addr = NULL;
> +			return true;
> +		}
> +		ha = ha->next;
> +	}
> +	return false;
> +}
> +
> +/*
>   * We implement a low memory allocator to allow us to allocate
>   * memory < 2G limit for the ACPICA table handling.  On 64 bit
>   * machines we have to ensure that cached copies of ACPI tables
> @@ -111,7 +197,6 @@ static void *fwts_low_mmap_walkdown(const size_t requested_size)
>  static void *fwts_low_mmap(const size_t requested_size)
>  {
>  	FILE *fp;
> -	char buffer[1024];
>  	char pathname[1024];
>  	void *addr_start;
>  	void *addr_end;
> @@ -129,15 +214,23 @@ static void *fwts_low_mmap(const size_t requested_size)
>  	if ((fp = fopen("/proc/self/maps", "r")) == NULL)
>  		return fwts_low_mmap_walkdown(requested_size);
>
> -	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
> -		if (sscanf(buffer, "%p-%p %*s %*x %*s %*u %1023s",
> +	while (!feof(fp)) {
> +		if (fscanf(fp, "%p-%p %*s %*x %*s %*u %1023s\n",
>  		    &addr_start, &addr_end, pathname) != 3)
>  			continue;
>  		/*
> +		 *  Sanity check data
> +		 */
> +		if ((addr_start <= (void *)LIMIT_START) ||
> +		    (addr_start >= (void *)LIMIT_2GB) ||
> +		    (addr_end <= (void *)LIMIT_START) ||
> +		    (addr_end >= (void *)LIMIT_2GB) ||
> +		    (addr_end <= addr_start))
> +			continue;
> +		/*
>  		 *  Try and allocate under first mmap'd address space
>  		 */
> -		if ((first_addr_start == NULL) &&
> -		    (addr_start > (void*)LIMIT_START)) {
> +		if (first_addr_start == NULL) {
>  			size_t sz = (requested_size + CHUNK_SIZE) & ~(CHUNK_SIZE - 1);
>  			uint8_t *addr = (uint8_t *)addr_start - sz;
>
> @@ -241,7 +334,14 @@ void *fwts_low_calloc(const size_t nmemb, const size_t size)
>  	hdr->size = n;
>  	hdr->magic = FWTS_ALLOC_MAGIC;
>
> -	return (void *)((uint8_t *)ret + sizeof(fwts_mmap_header));
> +	ret = (void *)((uint8_t *)ret + sizeof(fwts_mmap_header));
> +
> +	if (!hash_alloc_add(ret)) {
> +		munmap((void *)hdr, n);
> +		return NULL;
> +	}
> +
> +	return ret;
>  }
>
>  /*
> @@ -287,10 +387,23 @@ void *fwts_low_realloc(const void *ptr, const size_t size)
>   */
>  void fwts_low_free(const void *ptr)
>  {
> -	if (ptr) {
> -		fwts_mmap_header *hdr = (fwts_mmap_header *)
> -			((uint8_t *)ptr - sizeof(fwts_mmap_header));
> -		if (hdr->magic == FWTS_ALLOC_MAGIC)
> -			munmap(hdr, hdr->size);
> +	/*
> +	 *  Sanity check the address we are about to
> +	 *  try and free. If it is not known about
> +	 *  the don't free it.
> +	 */
> +	if (!ptr)
> +		return;
> +	if (!hash_alloc_remove(ptr)) {
> +		/* Should never happen... */
> +		fprintf(stderr, "double free on %p\n", ptr);
> +		return;
>  	}
> +
> +	fwts_mmap_header *hdr = (fwts_mmap_header *)
> +		((uint8_t *)ptr - sizeof(fwts_mmap_header));
> +
> +	/* Be doubly sure by checking magic before we munmap */
> +	if (hdr->magic == FWTS_ALLOC_MAGIC)
> +		munmap(hdr, hdr->size);
>  }
>

Acked-by: Ivan Hu <ivan.hu@canonical.com>
diff mbox

Patch

diff --git a/src/lib/src/fwts_alloc.c b/src/lib/src/fwts_alloc.c
index 09e7c93..1901a53 100644
--- a/src/lib/src/fwts_alloc.c
+++ b/src/lib/src/fwts_alloc.c
@@ -27,6 +27,92 @@ 
 #include "fwts.h"
 
 /*
+ *  Typically we use around 50 or so low mem allocations in fwts
+ *  so 509 is a good large prime size to ensure we get a good
+ *  hash table hit rate
+ */
+#define HASH_ALLOC_SIZE	(509)
+
+typedef struct hash_alloc {
+	void *addr;		/* allocation addr, NULL = not used */
+	struct hash_alloc *next;/* next one in hash chain */
+} hash_alloc_t;
+
+static hash_alloc_t *hash_allocs[HASH_ALLOC_SIZE];
+
+/*
+ *  hash_addr()
+ *	turn a void * address into a hash index
+ */
+static inline size_t hash_addr(const void *addr)
+{
+	ptrdiff_t h = (ptrdiff_t)addr;
+
+	h ^= h >> 17;
+	h %= HASH_ALLOC_SIZE;
+
+	return (size_t)h;
+}
+
+/*
+ *  hash_alloc_add()
+ *	add a allocated address into the hash of
+ *	known allocations so we can keep track
+ *	of valid allocs.
+ */
+static bool hash_alloc_add(void *addr)
+{
+	size_t h = hash_addr(addr);
+	hash_alloc_t *new = hash_allocs[h];
+
+	while (new) {
+		/* re-use old nullified records */
+		if (new->addr == NULL) {
+			/* old and free, so re-use */
+			new->addr = addr;
+			return true;
+		}
+		/* something is wrong, already in use */
+		if (new->addr == addr) {
+			return false;
+		}
+		new = new->next;
+	}
+
+	/* not found, add a new record */
+	new = malloc(sizeof(*new));
+	if (!new)
+		return false;
+
+	new->addr = addr;
+	new->next = hash_allocs[h];
+	hash_allocs[h] = new;
+
+	return true;
+}
+
+/*
+ *  hash_alloc_remove()
+ *	remove an allocated address from the hash
+ *	of know allocations.
+ */
+static bool hash_alloc_remove(const void *addr)
+{
+	size_t h = hash_addr(addr);
+	hash_alloc_t *ha = hash_allocs[h];
+
+	while (ha) {
+		if (ha->addr == addr) {
+			/* Just nullify it */
+			ha->addr = NULL;
+			return true;
+		}
+		ha = ha->next;
+	}
+	return false;
+}
+
+/*
  * We implement a low memory allocator to allow us to allocate
  * memory < 2G limit for the ACPICA table handling.  On 64 bit
  * machines we have to ensure that cached copies of ACPI tables
@@ -111,7 +197,6 @@  static void *fwts_low_mmap_walkdown(const size_t requested_size)
 static void *fwts_low_mmap(const size_t requested_size)
 {
 	FILE *fp;
-	char buffer[1024];
 	char pathname[1024];
 	void *addr_start;
 	void *addr_end;
@@ -129,15 +214,23 @@  static void *fwts_low_mmap(const size_t requested_size)
 	if ((fp = fopen("/proc/self/maps", "r")) == NULL)
 		return fwts_low_mmap_walkdown(requested_size);
 
-	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
-		if (sscanf(buffer, "%p-%p %*s %*x %*s %*u %1023s",
+	while (!feof(fp)) {
+		if (fscanf(fp, "%p-%p %*s %*x %*s %*u %1023s\n",
 		    &addr_start, &addr_end, pathname) != 3)
 			continue;
 		/*
+		 *  Sanity check data
+		 */
+		if ((addr_start <= (void *)LIMIT_START) ||
+		    (addr_start >= (void *)LIMIT_2GB) ||
+		    (addr_end <= (void *)LIMIT_START) ||
+		    (addr_end >= (void *)LIMIT_2GB) ||
+		    (addr_end <= addr_start))
+			continue;
+		/*
 		 *  Try and allocate under first mmap'd address space
 		 */
-		if ((first_addr_start == NULL) &&
-		    (addr_start > (void*)LIMIT_START)) {
+		if (first_addr_start == NULL) {
 			size_t sz = (requested_size + CHUNK_SIZE) & ~(CHUNK_SIZE - 1);
 			uint8_t *addr = (uint8_t *)addr_start - sz;
 
@@ -241,7 +334,14 @@  void *fwts_low_calloc(const size_t nmemb, const size_t size)
 	hdr->size = n;
 	hdr->magic = FWTS_ALLOC_MAGIC;
 
-	return (void *)((uint8_t *)ret + sizeof(fwts_mmap_header));
+	ret = (void *)((uint8_t *)ret + sizeof(fwts_mmap_header));
+
+	if (!hash_alloc_add(ret)) {
+		munmap((void *)hdr, n);
+		return NULL;
+	}
+
+	return ret;
 }
 
 /*
@@ -287,10 +387,23 @@  void *fwts_low_realloc(const void *ptr, const size_t size)
  */
 void fwts_low_free(const void *ptr)
 {
-	if (ptr) {
-		fwts_mmap_header *hdr = (fwts_mmap_header *)
-			((uint8_t *)ptr - sizeof(fwts_mmap_header));
-		if (hdr->magic == FWTS_ALLOC_MAGIC)
-			munmap(hdr, hdr->size);
+	/*
+	 *  Sanity check the address we are about to
+	 *  try and free. If it is not known about
+	 *  the don't free it.
+	 */
+	if (!ptr)
+		return;
+	if (!hash_alloc_remove(ptr)) {
+		/* Should never happen... */
+		fprintf(stderr, "double free on %p\n", ptr);
+		return;
 	}
+
+	fwts_mmap_header *hdr = (fwts_mmap_header *)
+		((uint8_t *)ptr - sizeof(fwts_mmap_header));
+
+	/* Be doubly sure by checking magic before we munmap */
+	if (hdr->magic == FWTS_ALLOC_MAGIC)
+		munmap(hdr, hdr->size);
 }