From 6c0d0cbab02d0fb13e48aacef7e26ae613ce7dae Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Mon, 27 Aug 2012 04:16:28 +0000 Subject: [PATCH] [AFD] - Fix one of the worst bugs in AFD of all time. Our AFD code operated under the assumption that none of the input parameters would change once we called LockRequest. This assumption is completely false. The only guarantee that made was that the pages never disappeared from us, not they they couldn't be modified. There are frequent cases where the user-mode buffer was modified from underneath us (WSPRecv allocates the struct on stack which makes it invalid during overlapped operations that complete later). When this happened, we would bugcheck when we tried to unlock the buffers since we accessed this in a member of the struct the caller passed us. - I've fixed this by adding a parameter to LockRequest which specifies whether the buffer should be copied back when it is unlocked. - This bug has been around for ages and I was never able to figure out why we just freed garbage sometimes. Now that the ws2_32_winetest exposed it reliably, I was finally able to fix it. svn path=/trunk/; revision=57175 --- reactos/drivers/network/afd/afd/bind.c | 2 +- reactos/drivers/network/afd/afd/connect.c | 10 ++--- reactos/drivers/network/afd/afd/context.c | 2 +- reactos/drivers/network/afd/afd/info.c | 4 +- reactos/drivers/network/afd/afd/listen.c | 2 +- reactos/drivers/network/afd/afd/lock.c | 48 ++++++++++++++++++++++- reactos/drivers/network/afd/afd/main.c | 12 +++--- reactos/drivers/network/afd/afd/read.c | 4 +- reactos/drivers/network/afd/afd/select.c | 4 +- reactos/drivers/network/afd/afd/write.c | 6 +-- reactos/drivers/network/afd/include/afd.h | 2 +- 11 files changed, 70 insertions(+), 26 deletions(-) diff --git a/reactos/drivers/network/afd/afd/bind.c b/reactos/drivers/network/afd/afd/bind.c index bafba1a624e..89ab22f776f 100644 --- a/reactos/drivers/network/afd/afd/bind.c +++ b/reactos/drivers/network/afd/afd/bind.c @@ -81,7 +81,7 @@ AfdBindSocket(PDEVICE_OBJECT DeviceObject, PIRP Irp, AFD_DbgPrint(MID_TRACE,("Called\n")); if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp ); - if( !(BindReq = LockRequest( Irp, IrpSp )) ) + if( !(BindReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 ); diff --git a/reactos/drivers/network/afd/afd/connect.c b/reactos/drivers/network/afd/afd/connect.c index f25c769e8cc..31c21a9f130 100644 --- a/reactos/drivers/network/afd/afd/connect.c +++ b/reactos/drivers/network/afd/afd/connect.c @@ -44,7 +44,7 @@ AfdSetConnectOptions(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID ConnectOptions = LockRequest(Irp, IrpSp); + PVOID ConnectOptions = LockRequest(Irp, IrpSp, FALSE); UINT ConnectOptionsSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -80,7 +80,7 @@ AfdSetConnectOptionsSize(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PUINT ConnectOptionsSize = LockRequest(Irp, IrpSp); + PUINT ConnectOptionsSize = LockRequest(Irp, IrpSp, FALSE); UINT BufferSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -144,7 +144,7 @@ AfdSetConnectData(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID ConnectData = LockRequest(Irp, IrpSp); + PVOID ConnectData = LockRequest(Irp, IrpSp, FALSE); UINT ConnectDataSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -179,7 +179,7 @@ AfdSetConnectDataSize(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PUINT ConnectDataSize = LockRequest(Irp, IrpSp); + PUINT ConnectDataSize = LockRequest(Irp, IrpSp, FALSE); UINT BufferSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -406,7 +406,7 @@ AfdStreamSocketConnect(PDEVICE_OBJECT DeviceObject, PIRP Irp, AFD_DbgPrint(MID_TRACE,("Called on %x\n", FCB)); if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp ); - if( !(ConnectReq = LockRequest( Irp, IrpSp )) ) + if( !(ConnectReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 ); diff --git a/reactos/drivers/network/afd/afd/context.c b/reactos/drivers/network/afd/afd/context.c index 112997b923f..ff5c2700dc6 100644 --- a/reactos/drivers/network/afd/afd/context.c +++ b/reactos/drivers/network/afd/afd/context.c @@ -60,7 +60,7 @@ AfdSetContext( PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp ) { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID Context = LockRequest(Irp, IrpSp); + PVOID Context = LockRequest(Irp, IrpSp, FALSE); if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp ); diff --git a/reactos/drivers/network/afd/afd/info.c b/reactos/drivers/network/afd/afd/info.c index 2567d98a388..a1f33bb705a 100644 --- a/reactos/drivers/network/afd/afd/info.c +++ b/reactos/drivers/network/afd/afd/info.c @@ -13,7 +13,7 @@ NTSTATUS NTAPI AfdGetInfo( PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp ) { NTSTATUS Status = STATUS_SUCCESS; - PAFD_INFO InfoReq = LockRequest(Irp, IrpSp); + PAFD_INFO InfoReq = LockRequest(Irp, IrpSp, TRUE); PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; PLIST_ENTRY CurrentEntry; @@ -99,7 +99,7 @@ NTSTATUS NTAPI AfdSetInfo( PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp ) { NTSTATUS Status = STATUS_SUCCESS; - PAFD_INFO InfoReq = LockRequest(Irp, IrpSp); + PAFD_INFO InfoReq = LockRequest(Irp, IrpSp, FALSE); PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; PCHAR NewBuffer; diff --git a/reactos/drivers/network/afd/afd/listen.c b/reactos/drivers/network/afd/afd/listen.c index 822cbab6821..c1fc5ff2ceb 100644 --- a/reactos/drivers/network/afd/afd/listen.c +++ b/reactos/drivers/network/afd/afd/listen.c @@ -217,7 +217,7 @@ NTSTATUS AfdListenSocket( PDEVICE_OBJECT DeviceObject, PIRP Irp, if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp ); - if( !(ListenReq = LockRequest( Irp, IrpSp )) ) + if( !(ListenReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 ); diff --git a/reactos/drivers/network/afd/afd/lock.c b/reactos/drivers/network/afd/afd/lock.c index 40af39bbb9a..2a6ad44dbe1 100644 --- a/reactos/drivers/network/afd/afd/lock.c +++ b/reactos/drivers/network/afd/afd/lock.c @@ -12,12 +12,13 @@ PVOID GetLockedData(PIRP Irp, PIO_STACK_LOCATION IrpSp) { ASSERT(Irp->MdlAddress); + ASSERT(Irp->Tail.Overlay.DriverContext[0]); - return MmGetSystemAddressForMdlSafe(Irp->MdlAddress, NormalPagePriority); + return Irp->Tail.Overlay.DriverContext[0]; } /* Lock a method_neither request so it'll be available from DISPATCH_LEVEL */ -PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ) { +PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp, BOOLEAN Output ) { BOOLEAN LockFailed = FALSE; ASSERT(!Irp->MdlAddress); @@ -84,12 +85,55 @@ PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ) { return NULL; } + /* The mapped address goes in index 1 */ + Irp->Tail.Overlay.DriverContext[1] = MmGetSystemAddressForMdlSafe(Irp->MdlAddress, NormalPagePriority); + if (!Irp->Tail.Overlay.DriverContext[1]) + { + AFD_DbgPrint(MIN_TRACE,("Failed to get mapped address\n")); + MmUnlockPages(Irp->MdlAddress); + IoFreeMdl( Irp->MdlAddress ); + Irp->MdlAddress = NULL; + return NULL; + } + + /* The allocated address goes in index 0 */ + Irp->Tail.Overlay.DriverContext[0] = ExAllocatePool(NonPagedPool, MmGetMdlByteCount(Irp->MdlAddress)); + if (!Irp->Tail.Overlay.DriverContext[0]) + { + AFD_DbgPrint(MIN_TRACE,("Failed to allocate memory\n")); + MmUnlockPages(Irp->MdlAddress); + IoFreeMdl( Irp->MdlAddress ); + Irp->MdlAddress = NULL; + return NULL; + } + + RtlCopyMemory(Irp->Tail.Overlay.DriverContext[0], + Irp->Tail.Overlay.DriverContext[1], + MmGetMdlByteCount(Irp->MdlAddress)); + + /* If we don't want a copy back, we zero the mapped address pointer */ + if (!Output) + { + Irp->Tail.Overlay.DriverContext[1] = NULL; + } + return GetLockedData(Irp, IrpSp); } VOID UnlockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ) { ASSERT(Irp->MdlAddress); + ASSERT(Irp->Tail.Overlay.DriverContext[0]); + + /* Check if we need to copy stuff back */ + if (Irp->Tail.Overlay.DriverContext[1] != NULL) + { + RtlCopyMemory(Irp->Tail.Overlay.DriverContext[1], + Irp->Tail.Overlay.DriverContext[0], + MmGetMdlByteCount(Irp->MdlAddress)); + } + + ExFreePool(Irp->Tail.Overlay.DriverContext[0]); MmUnlockPages( Irp->MdlAddress ); IoFreeMdl( Irp->MdlAddress ); Irp->MdlAddress = NULL; diff --git a/reactos/drivers/network/afd/afd/main.c b/reactos/drivers/network/afd/afd/main.c index fcbf9180df3..e7efc6d31b2 100644 --- a/reactos/drivers/network/afd/afd/main.c +++ b/reactos/drivers/network/afd/afd/main.c @@ -72,7 +72,7 @@ AfdSetDisconnectOptions(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID DisconnectOptions = LockRequest(Irp, IrpSp); + PVOID DisconnectOptions = LockRequest(Irp, IrpSp, FALSE); UINT DisconnectOptionsSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -108,7 +108,7 @@ AfdSetDisconnectOptionsSize(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PUINT DisconnectOptionsSize = LockRequest(Irp, IrpSp); + PUINT DisconnectOptionsSize = LockRequest(Irp, IrpSp, FALSE); UINT BufferSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -172,7 +172,7 @@ AfdSetDisconnectData(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PVOID DisconnectData = LockRequest(Irp, IrpSp); + PVOID DisconnectData = LockRequest(Irp, IrpSp, FALSE); UINT DisconnectDataSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -208,7 +208,7 @@ AfdSetDisconnectDataSize(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PUINT DisconnectDataSize = LockRequest(Irp, IrpSp); + PUINT DisconnectDataSize = LockRequest(Irp, IrpSp, FALSE); UINT BufferSize = IrpSp->Parameters.DeviceIoControl.InputBufferLength; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -244,7 +244,7 @@ AfdGetTdiHandles(PDEVICE_OBJECT DeviceObject, PIRP Irp, { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_FCB FCB = FileObject->FsContext; - PULONG HandleFlags = LockRequest(Irp, IrpSp); + PULONG HandleFlags = LockRequest(Irp, IrpSp, TRUE); PAFD_TDI_HANDLE_DATA HandleData = Irp->UserBuffer; if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); @@ -680,7 +680,7 @@ AfdDisconnect(PDEVICE_OBJECT DeviceObject, PIRP Irp, if (!SocketAcquireStateLock(FCB)) return LostSocket(Irp); - if (!(DisReq = LockRequest(Irp, IrpSp))) + if (!(DisReq = LockRequest(Irp, IrpSp, FALSE))) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 ); diff --git a/reactos/drivers/network/afd/afd/read.c b/reactos/drivers/network/afd/afd/read.c index b45a96e51f8..32697f68c57 100644 --- a/reactos/drivers/network/afd/afd/read.c +++ b/reactos/drivers/network/afd/afd/read.c @@ -445,7 +445,7 @@ AfdConnectedSocketReadData(PDEVICE_OBJECT DeviceObject, PIRP Irp, Irp, 0 ); } - if( !(RecvReq = LockRequest( Irp, IrpSp )) ) + if( !(RecvReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 ); @@ -715,7 +715,7 @@ AfdPacketSocketReadData(PDEVICE_OBJECT DeviceObject, PIRP Irp, return UnlockAndMaybeComplete(FCB, STATUS_FILE_CLOSED, Irp, 0); } - if( !(RecvReq = LockRequest( Irp, IrpSp )) ) + if( !(RecvReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete(FCB, STATUS_NO_MEMORY, Irp, 0); AFD_DbgPrint(MID_TRACE,("Recv flags %x\n", RecvReq->AfdFlags)); diff --git a/reactos/drivers/network/afd/afd/select.c b/reactos/drivers/network/afd/afd/select.c index 2ab6329da79..71d914beed3 100644 --- a/reactos/drivers/network/afd/afd/select.c +++ b/reactos/drivers/network/afd/afd/select.c @@ -259,7 +259,7 @@ AfdEventSelect( PDEVICE_OBJECT DeviceObject, PIRP Irp, PFILE_OBJECT FileObject = IrpSp->FileObject; NTSTATUS Status = STATUS_NO_MEMORY; PAFD_EVENT_SELECT_INFO EventSelectInfo = - (PAFD_EVENT_SELECT_INFO)LockRequest( Irp, IrpSp ); + (PAFD_EVENT_SELECT_INFO)LockRequest( Irp, IrpSp, FALSE ); PAFD_FCB FCB = FileObject->FsContext; if( !SocketAcquireStateLock( FCB ) ) { @@ -322,7 +322,7 @@ AfdEnumEvents( PDEVICE_OBJECT DeviceObject, PIRP Irp, PIO_STACK_LOCATION IrpSp ) { PFILE_OBJECT FileObject = IrpSp->FileObject; PAFD_ENUM_NETWORK_EVENTS_INFO EnumReq = - (PAFD_ENUM_NETWORK_EVENTS_INFO)LockRequest( Irp, IrpSp ); + (PAFD_ENUM_NETWORK_EVENTS_INFO)LockRequest( Irp, IrpSp, TRUE ); PAFD_FCB FCB = FileObject->FsContext; AFD_DbgPrint(MID_TRACE,("Called (FCB %x)\n", FCB)); diff --git a/reactos/drivers/network/afd/afd/write.c b/reactos/drivers/network/afd/afd/write.c index 15738bd057c..84025d38e48 100644 --- a/reactos/drivers/network/afd/afd/write.c +++ b/reactos/drivers/network/afd/afd/write.c @@ -299,7 +299,7 @@ AfdConnectedSocketWriteData(PDEVICE_OBJECT DeviceObject, PIRP Irp, 0 ); } - if( !(SendReq = LockRequest( Irp, IrpSp )) ) + if( !(SendReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete( FCB, STATUS_NO_MEMORY, Irp, 0 ); /* Must lock buffers before handing off user data */ @@ -369,7 +369,7 @@ AfdConnectedSocketWriteData(PDEVICE_OBJECT DeviceObject, PIRP Irp, return UnlockAndMaybeComplete(FCB, STATUS_FILE_CLOSED, Irp, 0); } - if( !(SendReq = LockRequest( Irp, IrpSp )) ) + if( !(SendReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete ( FCB, STATUS_NO_MEMORY, Irp, TotalBytesCopied ); @@ -516,7 +516,7 @@ AfdPacketSocketWriteData(PDEVICE_OBJECT DeviceObject, PIRP Irp, return UnlockAndMaybeComplete(FCB, STATUS_FILE_CLOSED, Irp, 0); } - if( !(SendReq = LockRequest( Irp, IrpSp )) ) + if( !(SendReq = LockRequest( Irp, IrpSp, FALSE )) ) return UnlockAndMaybeComplete(FCB, STATUS_NO_MEMORY, Irp, 0); if (FCB->State == SOCKET_STATE_CREATED) diff --git a/reactos/drivers/network/afd/include/afd.h b/reactos/drivers/network/afd/include/afd.h index e55fffe2281..f8208002f6c 100644 --- a/reactos/drivers/network/afd/include/afd.h +++ b/reactos/drivers/network/afd/include/afd.h @@ -308,7 +308,7 @@ VOID SocketStateUnlock( PAFD_FCB FCB ); NTSTATUS LostSocket( PIRP Irp ); PAFD_HANDLE LockHandles( PAFD_HANDLE HandleArray, UINT HandleCount ); VOID UnlockHandles( PAFD_HANDLE HandleArray, UINT HandleCount ); -PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ); +PVOID LockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp, BOOLEAN Output ); VOID UnlockRequest( PIRP Irp, PIO_STACK_LOCATION IrpSp ); PVOID GetLockedData( PIRP Irp, PIO_STACK_LOCATION IrpSp ); NTSTATUS LeaveIrpUntilLater( PAFD_FCB FCB, PIRP Irp, UINT Function );