⬢ DragonFlyBSD Kernel Audit
DF-5663 / fix.diff
← back to finding ↓ download raw
--- sys/bus/cam/cam_extend.c.orig
+++ sys/bus/cam/cam_extend.c
@@ -4,9 +4,9 @@
  *
  * TRW Financial Systems, in accordance with their agreement with Carnegie
  * Mellon University, makes this software available to CMU to distribute
- * or use in any manner that they see fit as long as this message is kept with
- * the software. For this reason TFS also grants any other persons or
- * organisations permission to use or modify this software.
+ * or use in any manner that they see fit as long as this message is kept
+ * with the software.  For this reason TFS also grants any other persons
+ * or organisations permission to use or modify this software.
  *
  * TFS supplies this software to be publicly redistributed
  * on the understanding that TFS is not responsible for the correct
@@ -27,6 +27,8 @@
 #include <sys/systm.h>
 #include <sys/kernel.h>
 #include <sys/malloc.h>
+#include <sys/spinlock2.h>
+#include <sys/limits.h>
 
 #include "cam_extend.h"
 
@@ -34,6 +36,7 @@
 {
 	int nelem;
 	void **ps;
+	struct spinlock ex_lock;
 };
 
 /* EXTEND_CHUNK: Number of extend slots to allocate whenever we need a new
@@ -46,51 +49,99 @@
 struct extend_array *
 cam_extend_new(void)
 {
-	return(kmalloc(sizeof(struct extend_array), M_DEVBUF,
-	    M_INTWAIT | M_ZERO));
+	struct extend_array *ea;
+
+	ea = kmalloc(sizeof(struct extend_array), M_DEVBUF,
+	    M_INTWAIT | M_ZERO);
+	spin_init(&ea->ex_lock, "cam_extend");
+	return (ea);
 }
 
+/*
+ * Returns the stored value on success, NULL on failure (bad index,
+ * allocation failure, or the slot already has storage).
+ *
+ * NOTE: The index is fully validated.  The old code only checked the
+ *	 upper bound, so any negative index indexed before the array
+ *	 (an out-of-bounds kernel write) or faulted on a NULL ps.
+ *
+ *	 The array is also synchronized now: growth frees the old ps[]
+ *	 chunk, which racing lockless cam_extend_get() readers could
+ *	 still dereference (use-after-free read handed back as the
+ *	 stored object pointer).
+ */
 void *
 cam_extend_set(struct extend_array *ea, int index, void *value)
 {
+	void **space, **oldps;
+
+	if (index < 0 || index > INT_MAX - EXTEND_CHUNK)
+		return (NULL);
+
 	if (index >= ea->nelem) {
-		void **space;
 		space = kmalloc(sizeof(void *) * (index + EXTEND_CHUNK),
 		    M_DEVBUF, M_INTWAIT | M_ZERO);
+		if (space == NULL)
+			return (NULL);
 
-		/* Make sure we have something to copy before we copy it */
-		if (ea->nelem) {
-			bcopy(ea->ps, space, sizeof(void *) * ea->nelem);
-			kfree(ea->ps, M_DEVBUF);
+		/*
+		 * Re-check under the lock; the allocation was done
+		 * unlocked because M_INTWAIT may block.
+		 */
+		spin_lock(&ea->ex_lock);
+		if (index >= ea->nelem) {
+			/* Make sure we have something to copy before we
+			 * copy it */
+			if (ea->nelem) {
+				bcopy(ea->ps, space,
+				      sizeof(void *) * ea->nelem);
+			}
+			oldps = ea->ps;
+			ea->ps = space;
+			ea->nelem = index + EXTEND_CHUNK;
+			spin_unlock(&ea->ex_lock);
+			if (oldps != NULL)
+				kfree(oldps, M_DEVBUF);
+		} else {
+			spin_unlock(&ea->ex_lock);
+			kfree(space, M_DEVBUF);
 		}
-
-		ea->ps = space;
-		ea->nelem = index + EXTEND_CHUNK;
 	}
+
+	spin_lock(&ea->ex_lock);
 	if (ea->ps[index]) {
+		spin_unlock(&ea->ex_lock);
 		kprintf("extend_set: entry %d already has storage.\n", index);
-		return 0;
+		return (NULL);
 	}
-	else
-		ea->ps[index] = value;
+	ea->ps[index] = value;
+	spin_unlock(&ea->ex_lock);
 
-	return value;
+	return (value);
 }
 
 void *
 cam_extend_get(struct extend_array *ea,
 	       int index)
 {
+	void *p;
+
 	if (ea == NULL || index >= ea->nelem || index < 0)
 		return NULL;
-	return ea->ps[index];
+	spin_lock(&ea->ex_lock);
+	p = ea->ps[index];
+	spin_unlock(&ea->ex_lock);
+	return (p);
 }
 
 void
 cam_extend_release(struct extend_array *ea, int index)
 {
-	void *p = cam_extend_get(ea, index);
-	if (p) {
+	if (ea == NULL || index >= ea->nelem || index < 0)
+		return;
+	spin_lock(&ea->ex_lock);
+	if (ea->ps[index] != NULL) {
 		ea->ps[index] = NULL;
 	}
+	spin_unlock(&ea->ex_lock);
 }