Avoid double-insert and other corruption in BpfPrograms

The primary cause was the fact that the transition to OpenRunning state
involves inserting the OPEN_INSTANCE's BpfProgram into the list without
checking for whether it had already been inserted. The BIOCSETF handler
was inserting it at OpenAttached, which could lead to double-insertion.

This addresses #831. Additional checks for corruption have been added to
protect against regressions.

Author: bonsaiviking

packetWin7/npf/npf/Openclos.c

@@ -562,45 +562,44 @@ _Use_decl_annotations_
 VOID
 NPF_RegisterBpf(
 	PNPCAP_FILTER_MODULE pFiltMod,
-	PNPCAP_BPF_PROGRAM pBpfProgram,
-	PNPCAP_BPF_PROGRAM pOldBpfProgram)
+	PNPCAP_BPF_PROGRAM pBpfProgram)
 {
 	// Assert/verify pBpfProgram fields are set
 	LOCK_STATE_EX lockState;
 
 	NT_ASSERT(pBpfProgram->pOpen != NULL);
-	NT_ASSERT(pBpfProgram->BpfProgramsEntry.Flink == NULL
-			&& pBpfProgram->BpfProgramsEntry.Blink == NULL);
+	NT_ASSERT(pBpfProgram->pOpen->pFiltMod == pFiltMod);
 
-	// Lock the BpfPrograms list
-	NdisAcquireRWLockWrite(pFiltMod->BpfProgramsLock, &lockState, 0);
-	// Insert/update the bpf for this open instance
-	if (pOldBpfProgram != NULL)
-	{
-		// This Open has a program in the list already.
-		NT_ASSERT(pOldBpfProgram != pBpfProgram);
-		NT_ASSERT(pBpfProgram->pOpen == pOldBpfProgram->pOpen);
-		PLIST_ENTRY pOldEntry = &pOldBpfProgram->BpfProgramsEntry;
-		PLIST_ENTRY pNewEntry = &pBpfProgram->BpfProgramsEntry;
-
-		// Link the new program to the rest of the list
-		pNewEntry->Flink = pOldEntry->Flink;
-		pNewEntry->Blink = pOldEntry->Blink;
-		// Link the next and previous entries to the new program
-		pOldEntry->Flink->Blink = pNewEntry;
-		pOldEntry->Blink->Flink = pNewEntry;
-		// Make the old program invalid
-		// Freeing the program is the caller's responsibility.
-		pOldEntry->Flink = NULL;
-		pOldEntry->Blink = NULL;
-	}
-	else
+	PLIST_ENTRY pNewEntry = &pBpfProgram->BpfProgramsEntry;
+	// If the program is already in the list, nothing more to do
+	if (pNewEntry->Flink != NULL && NT_VERIFY(pNewEntry->Blink != NULL))
 	{
-		InsertTailList(&pFiltMod->BpfPrograms, &pBpfProgram->BpfProgramsEntry);
+		// In debug mode we can take the time to verify
+#if defined(_DBG)
+		for (PLIST_ENTRY Curr = pFiltMod->BpfPrograms.Flink;
+				Curr != &pFiltMod->BpfPrograms;
+				Curr = Curr->Flink)
+		{
+			if (Curr == pNewEntry)
+			{
+				return;
+			}
+		}
+		NT_ASSERT(FALSE);
+#endif
+		return;
 	}
+
+	NT_ASSERT(pNewEntry->Flink == NULL && pNewEntry->Blink == NULL);
+
+	// Lock the BpfPrograms list
+	NdisAcquireRWLockWrite(pFiltMod->BpfProgramsLock, &lockState, 0);
+	// Insert the bpf for this open instance
+	InsertTailList(&pFiltMod->BpfPrograms, pNewEntry);
 	// Unlock the list
 	NdisReleaseRWLock(pFiltMod->BpfProgramsLock, &lockState);
 }
+
 _Use_decl_annotations_
 VOID
 NPF_UnregisterBpf(
@@ -617,6 +616,21 @@ NPF_UnregisterBpf(
 	NT_ASSERT(pBpfProgram->BpfProgramsEntry.Blink != NULL);
 	// Lock the BpfPrograms list
 	NdisAcquireRWLockWrite(pFiltMod->BpfProgramsLock, &lockState, 0);
+	// In debug mode we can take the time to verify
+#if defined(_DBG)
+	BOOLEAN bFound = FALSE;
+	for (PLIST_ENTRY Curr = pFiltMod->BpfPrograms.Flink;
+			Curr != &pFiltMod->BpfPrograms;
+			Curr = Curr->Flink)
+	{
+		if (Curr == &pBpfProgram->BpfProgramsEntry)
+		{
+			bFound = TRUE;
+			break;
+		}
+	}
+	NT_ASSERT(bFound);
+#endif
 	// remove the bpf for this open instance
 	RemoveEntryList(&pBpfProgram->BpfProgramsEntry);
 	// Unlock the list
@@ -700,7 +714,7 @@ NPF_StartUsingOpenInstance(
 				NPF_UpdateTimestampModeCounts(pOpen->pFiltMod, pOpen->TimestampMode, TIMESTAMPMODE_UNSET);
 
 				// Insert a null filter (accept all)
-				NPF_RegisterBpf(pOpen->pFiltMod, pOpen->BpfProgram, NULL);
+				NPF_RegisterBpf(pOpen->pFiltMod, pOpen->BpfProgram);
 				pOpen->OpenStatus = OpenRunning;
 			}
 			else
@@ -2265,7 +2279,7 @@ NPF_AttachAdapter(
 				if (pOpen->ReattachStatus < OpenAttached)
 				{
 					NPF_UpdateTimestampModeCounts(pFiltMod, pOpen->TimestampMode, TIMESTAMPMODE_UNSET);
-					NPF_RegisterBpf(pOpen->pFiltMod, pOpen->BpfProgram, NULL);
+					NPF_RegisterBpf(pOpen->pFiltMod, pOpen->BpfProgram);
 				}
 				pOpen->OpenStatus = pOpen->ReattachStatus;
 				Curr = PopEntryList(&ReattachOpens);

packetWin7/npf/npf/Packet.c

@@ -1187,11 +1187,12 @@ static NTSTATUS funcBIOCSETF(_In_ POPEN_INSTANCE pOpen,
 	TmpBPFProgram->pOpen = pOpen;
 
 	PVOID pOld = InterlockedExchangePointer(&pOpen->BpfProgram, TmpBPFProgram);
+	NPF_UnregisterBpf(pOpen->pFiltMod, pOld);
 
-	// Register the new BPF program if the Open is Attached
-	if (NPF_StartUsingOpenInstance(pOpen, OpenAttached, NPF_IRQL_UNKNOWN)) {
-		NPF_RegisterBpf(pOpen->pFiltMod, TmpBPFProgram, pOld);
-		NPF_StopUsingOpenInstance(pOpen, OpenAttached, NPF_IRQL_UNKNOWN);
+	// Register the new BPF program if the Open is Running
+	if (NPF_StartUsingOpenInstance(pOpen, OpenRunning, NPF_IRQL_UNKNOWN)) {
+		NPF_RegisterBpf(pOpen->pFiltMod, TmpBPFProgram);
+		NPF_StopUsingOpenInstance(pOpen, OpenRunning, NPF_IRQL_UNKNOWN);
 	}
 	else {
 		// After InterlockedExchangePointer above, the memory at TmpBPFProgram

packetWin7/npf/npf/Packet.h

@@ -398,8 +398,7 @@ NPCAP_BPF_PROGRAM, *PNPCAP_BPF_PROGRAM;
 VOID
 NPF_RegisterBpf(
 	_In_ PNPCAP_FILTER_MODULE pFiltMod,
-	_In_ __drv_aliasesMem PNPCAP_BPF_PROGRAM pBpfProgram,
-	_In_opt_ PNPCAP_BPF_PROGRAM pOldBpfProgram);
+	_In_ __drv_aliasesMem PNPCAP_BPF_PROGRAM pBpfProgram);
 VOID
 NPF_UnregisterBpf(
 	_In_ PNPCAP_FILTER_MODULE pFiltMod,