BLASTing Linux Code

Jan Tobias Mühlberg and Gerald Lüttgen
Department of Computer Science, University of York, York YO10 5DD, U.K.


main page | next example


Commit Overview | Files | Comments

Checking Memory Safety: Example 3


Commit Overview

Commit Key d0743a5b7b837334cb414b773529d51de3de0471
Subject [PATCH] drivers/scsi/dpt_i2o.c: fix a user-after-free
Description The Coverity checker spotted this obvious use-after-free
Requires Linux 2.6.14 kernel source as from git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/linux-2.6.14.y.git

--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -816,7 +816,7 @@ static int adpt_hba_reset(adpt_hba* pHba
static void adpt_i2o_sys_shutdown(void)
{
adpt_hba *pHba, *pNext;
- struct adpt_i2o_post_wait_data *p1, *p2;
+ struct adpt_i2o_post_wait_data *p1, *old;
printk(KERN_INFO"Shutting down Adaptec I2O controllers.\n");
printk(KERN_INFO" This could take a few minutes if there are many devices attached\n");
@@ -830,13 +830,14 @@ static void adpt_i2o_sys_shutdown(void)
}
/* Remove any timedout entries from the wait queue. */
- p2 = NULL;
// spin_lock_irqsave(&adpt_post_wait_lock, flags);
/* Nothing should be outstanding at this point so just
* free them
*/
- for(p1 = adpt_post_wait_queue; p1; p2 = p1, p1 = p2->next) {
- kfree(p1);
+ for(p1 = adpt_post_wait_queue; p1;) {
+ old = p1;
+ p1 = p1->next;
+ kfree(old);
}
// spin_unlock_irqrestore(&adpt_post_wait_lock, flags);
adpt_post_wait_queue = NULL;

(purple: line numbers and function names; red: line removed; green: line added)

Files

Unmodified sources

Comments

Despite being obvious, this programming error is quite difficult to verify. In some sense, the problem is similar to examples 1 and 2. In adpt_i2o_sys_shutdown() pointer is released using kfree() but de-referenced again afterwards. The bug resides in the for-loop in lines 838 to 840 of the source file. The loop is used to free a list of pointers to structs. Each of them contains a pointer next to the next element of the list. However, in the first cycle the loop stores the first element of this list in pointer p1, checks whether it does not equal NULL, makes p1 point to the next element and frees p1. In the next cycle it checks whether p1 still does not equal NULL and sets it to p1->next. This loop behaviour actually contains two errors: Firstly, the second element of the list is freed without checking whether it might equal NULL. Secondly, the already freed second element is de-referenced again in the second loop cycle.

Verifying a linked list package has already been attempted by Mong as referenced in our paper. We basically experienced similar shortcomings as he did. The major issue is, that BLAST is not able to keep track of a set of pointers. Probably some sort of a heap model would be required in order to make BLAST able to detect problems like this.

Source: http://www.kernel.org



Jan Tobias Mühlberg, $Date$