[PATCH] Fix "use after free" / "double free" bug in amd_create_gatt_pages / amd_free_gatt_pages

[Jesper Juhl]

Hi,

Coverity spotted a "use after free" bug in 
drivers/char/agp/amd-k7-agp.c::amd_create_gatt_pages().

The problem is this:
	If "entry = kzalloc(sizeof(struct amd_page_map), GFP_KERNEL);"
fails, then there's a loop in the function to free all entries 
allocated so far and break out of the allocation loop. That in itself 
is pretty sane, but then the (now freed) 'tables' is assigned to 
amd_irongate_private.gatt_pages and 'retval' is set to -ENOMEM which 
causes amd_free_gatt_pages(); to be called at the end of the function.
The problem with this is that amd_free_gatt_pages() will then loop 
'amd_irongate_private.num_tables' times and try to free each entry in 
tables[] - this is bad since tables has already been freed and 
furthermore it will call kfree(tables) at the end - a double free.

This patch removes the freeing loop in amd_create_gatt_pages() and 
instead relies entirely on the call to amd_free_gatt_pages() to free 
everything we allocated in case of an error. It also sets 
amd_irongate_private.num_tables to the actual number of entries 
allocated instead of just using the value passed in from the caller - 
this ensures that amd_free_gatt_pages() will only attempt to free 
stuff that was actually allocated.

Note: I'm in no way intimate with this code and I have no way to 
actually test this patch (besides compile test it), so while I've 
tried to be careful in reading the code and make sure the patch 
does the right thing an ACK from someone who actually knows the 
code in-depth would be very much appreciated.


Signed-off-by: Jesper Juhl <jesper.juhl@xxxxxxxxx>
---

 drivers/char/agp/amd-k7-agp.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
index df0ddf1..56d9c4b 100644
--- a/drivers/char/agp/amd-k7-agp.c
+++ b/drivers/char/agp/amd-k7-agp.c
@@ -100,21 +100,16 @@ static int amd_create_gatt_pages(int nr_tables)
 
 	for (i = 0; i < nr_tables; i++) {
 		entry = kzalloc(sizeof(struct amd_page_map), GFP_KERNEL);
+		tables[i] = entry;
 		if (entry == NULL) {
-			while (i > 0) {
-				kfree(tables[i-1]);
-				i--;
-			}
-			kfree(tables);
 			retval = -ENOMEM;
 			break;
 		}
-		tables[i] = entry;
 		retval = amd_create_page_map(entry);
 		if (retval != 0)
 			break;
 	}
-	amd_irongate_private.num_tables = nr_tables;
+	amd_irongate_private.num_tables = i;
 	amd_irongate_private.gatt_pages = tables;
 
 	if (retval != 0)


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/