kern/98015: [PATCH] bfe(4): double free in error handling path.

[Ulrich Spoerlein]

>Number:         98015
>Category:       kern
>Synopsis:       [PATCH] bfe(4): double free in error handling path.
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sat May 27 16:00:44 GMT 2006
>Closed-Date:
>Last-Modified:
>Originator:     Ulrich Spoerlein
>Release:        FreeBSD 6.1-STABLE i386
>Organization:
>Environment:
>Description:
When bfe_attach() fails, bfe_release_resources() is sometimes called twice.

>How-To-Repeat:
Change one if (foo()) to if (foo() || 1) and watch it panic.
>Fix:

Please note, that I really don't know what I'm doing here! I looked at various
other drivers in the tree, and they are all similiar yet slightly different. I'm
pretty sure, there are more faulty drivers due to copy'n'paste.

Since bfe_release_resource() is now only called from bfe_detach(), these two
could/should be merged. This would bring bfe(4) more in line with other NIC
drivers. There are also some style(9) "violations"

NB: Found by:	Coverity CID 476

Then, there was a lock order reversal on kldunload. It seems it has vanished,
though I'm really not sure right now.

--- syscall (444, FreeBSD ELF32, kldunloadf), eip = 0x480b45cb, esp = 0xbfbfe89c, ebp = 0xbfbfed08 ---
lock order reversal: (sleepable after non-sleepable)
 1st 0xc5020114 bfe0 (network driver) @ /usr/src/sys/modules/bfe/../../dev/bfe/if_bfe.c:446
 2nd 0xc08b3b40 ACPI root bus (ACPI root bus) @ /usr/src/sys/modules/acpi/acpi/../../../dev/acpica/acpi.c:1075
KDB: stack backtrace:
kdb_backtrace(0,ffffffff,c072b2c0,c072bc20,c06f2d8c) at kdb_backtrace+0x29
witness_checkorder(c08b3b40,9,c08ac97b,433) at witness_checkorder+0x578
_sx_xlock(c08b3b40,c08ac97b,433,c4b4cc00,c4dc1640) at _sx_xlock+0x50
acpi_release_resource(c4b4cc00,c4c28500,1,0,c4dc1640) at acpi_release_resource+0x23
bus_generic_release_resource(c4b4c700,c4c28500,1,0,c4dc1640) at bus_generic_release_resource+0x64
resource_list_release(c4c4cd04,c4c04980,c4c28500,1,0) at resource_list_release+0x6e
bus_generic_rl_release_resource(c4c04980,c4c28500,1,0,c4dc1640) at bus_generic_rl_release_resource+0x5e
bus_generic_release_resource(c4c04600,c4c28500,1,0,c4dc1640) at bus_generic_release_resource+0x64
resource_list_release(c4c4cd04,c4c28580,c4c28500,1,0) at resource_list_release+0xfb
bus_generic_rl_release_resource(c4c28580,c4c28500,1,0,c4dc1640) at bus_generic_rl_release_resource+0x5e
bus_release_resource(c4c28500,1,0,c4dc1640,c501e000) at bus_release_resource+0x61
bfe_release_resources(c4dc9c00,c4c28500,c4c28500,c4ef4800,ed0cdc30) at bfe_release_resources+0x142
bfe_detach(c4c28500) at bfe_detach+0x58
device_detach(c4c28500) at device_detach+0x70
devclass_delete_driver(c4b0a240,c501d6f0,c4dc1900,c4f6d700,0) at devclass_delete_driver+0x8c
driver_module_handler(c4dc1900,1,c501d6dc,c4dc1900,ed0cdcb0) at driver_module_handler+0xa5
module_unload(c4dc1900,0,c071c500,c06b0e72,1fb) at module_unload+0x37
linker_file_unload(c4f6d700,0,0,c4db1900,ed0cdcdc) at linker_file_unload+0x72
kern_kldunload(c4db1900,1d,0,ed0cdd30,c0678e17) at kern_kldunload+0x7c
kldunloadf(c4db1900,ed0cdd04,2,0,292) at kldunloadf+0x1e
syscall(3b,3b,3b,1d,bfbfee37) at syscall+0x22f
Xint0x80_syscall() at Xint0x80_syscall+0x1f
--- syscall (444, FreeBSD ELF32, kldunloadf), eip = 0x480b45cb, esp = 0xbfbfe89c, ebp = 0xbfbfed08 ---


--- if_bfe.patch begins here ---
--- if_bfe.c.orig	Tue May 23 19:49:54 2006
+++ if_bfe.c	Wed May 24 17:16:32 2006
@@ -369,7 +369,6 @@
 	if (bfe_dma_alloc(dev)) {
 		printf("bfe%d: failed to allocate DMA resources\n",
 		    sc->bfe_unit);
-		bfe_release_resources(sc);
 		error = ENXIO;
 		goto fail;
 	}
@@ -424,13 +423,14 @@
 			bfe_intr, sc, &sc->bfe_intrhand);
 
 	if (error) {
-		bfe_release_resources(sc);
 		printf("bfe%d: couldn't set up irq\n", unit);
+		ether_ifdetach(ifp);
 		goto fail;
 	}
 fail:
 	if (error)
-		bfe_release_resources(sc);
+		bfe_detach(dev);
+
 	return (error);
 }
 
@@ -443,24 +443,17 @@
 	sc = device_get_softc(dev);
 
 	KASSERT(mtx_initialized(&sc->bfe_mtx), ("bfe mutex not initialized"));
-	BFE_LOCK(sc);
-
 	ifp = sc->bfe_ifp;
 
 	if (device_is_attached(dev)) {
+		BFE_LOCK(sc);
 		bfe_stop(sc);
+		bfe_chip_reset(sc);
+		BFE_UNLOCK(sc);
 		ether_ifdetach(ifp);
 	}
 
-	bfe_chip_reset(sc);
-
-	bus_generic_detach(dev);
-	if(sc->bfe_miibus != NULL)
-		device_delete_child(dev, sc->bfe_miibus);
-
 	bfe_release_resources(sc);
-	BFE_UNLOCK(sc);
-	mtx_destroy(&sc->bfe_mtx);
 
 	return (0);
 }
@@ -922,6 +915,11 @@
 
 	dev = sc->bfe_dev;
 
+	if (sc->bfe_miibus != NULL)
+		device_delete_child(dev, sc->bfe_miibus);
+
+	bus_generic_detach(dev);
+
 	if (sc->bfe_vpd_prodname != NULL)
 		free(sc->bfe_vpd_prodname, M_DEVBUF);
 
@@ -971,6 +969,8 @@
 
 	if(sc->bfe_parent_tag != NULL)
 		bus_dma_tag_destroy(sc->bfe_parent_tag);
+
+	mtx_destroy(&sc->bfe_mtx);
 
 	return;
 }
--- if_bfe.patch ends here ---


>Release-Note:
>Audit-Trail:
>Unformatted: