From 1269f86c76c44f89ef363833d766a11ff2ab15ed Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Sat, 12 Dec 2020 15:20:11 +0100 Subject: [PATCH] driver: use partial MDL for slicing ring, rather than NB's DataOffset Providing the DataOffset member of the NBL allocation function or setting that member in the NB header indicates to NDIS not only that the data starts at that offset, but that there's that amount of space *available for it to use as it wants* before that offset. This meant that NDIS was allowed to scribble data before the packet. This was bounded by the size of the ring, so there was never any risk of memory corruption, and since the ring is shared by userspace as well as the rest of the kernel, we've always taken care of reading from it closely, checking all values, and erroring out on corruption of the ring. So, if NDIS wrote before the first packet, this would wind up corrupting the RingTail and Alertable fields of the ring. The receiver thread would then notice this, error out, and set the RingHead to MAXULONG on its way out the door, so that userspace can detect it. And indeed wintun.dll then started returning EOF from its write function. Mostly this was not an issue, because we're not expecting for data to be pushed on the head of a packet on ingress. But WSL2's Hyper-V driver is actually pushing an ethernet header onto the front of the packet before passing it off to Linux. Practically speaking, this manifested itself in the RingTail and Alertable fields having Linux's MAC address! And then the adapter would be EOF'd. This was reported as happening after WSL2 sends the *first* packet, but not others, which makes sense, because it has to be at the beginning in order to corrupt those fields. This fixes the problem by simply using a new MDL for the span we want, instead of using the misunderstood DataOffset field. In order to not need to keep track of memory allocations, we allocate the MDL as part of the NBL's context area. And in order to avoid additional mappings, we use IoBuildPartialMdl, which returns an MDL_PARTIAL, which does not have an additional mapping that needs to be freed or unmapped. After making this change, WSL2 no longer appears to halt the adapter, and all works well. Fixes: be8d2cb ("Avoid allocating second MDL") Signed-off-by: Jason A. Donenfeld --- driver/wintun.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/driver/wintun.c b/driver/wintun.c index 72178d4..a762dcb 100644 --- a/driver/wintun.c +++ b/driver/wintun.c @@ -507,11 +507,20 @@ TunProcessReceiveData(_Inout_ TUN_CTX *Ctx) break; RingHead = TUN_RING_WRAP(RingHead + AlignedPacketSize, RingCapacity); - - NET_BUFFER_LIST *Nbl = NdisAllocateNetBufferAndNetBufferList( - Ctx->NblPool, 0, 0, Ctx->Device.Receive.Mdl, (ULONG)(Packet->Data - (UCHAR *)Ring), PacketSize); + MDL *Mdl; + NET_BUFFER_LIST *Nbl = NdisAllocateNetBufferAndNetBufferList(Ctx->NblPool, sizeof(*Mdl), 0, NULL, 0, 0); if (!Nbl) goto skipNbl; + Mdl = (MDL *)NET_BUFFER_LIST_CONTEXT_DATA_START(Nbl); + IoBuildPartialMdl( + Ctx->Device.Receive.Mdl, + Mdl, + (UCHAR *)MmGetMdlVirtualAddress(Ctx->Device.Receive.Mdl) + (ULONG)(Packet->Data - (UCHAR *)Ring), + PacketSize); + NET_BUFFER *Nb = NET_BUFFER_LIST_FIRST_NB(Nbl); + NET_BUFFER_FIRST_MDL(Nb) = NET_BUFFER_CURRENT_MDL(Nb) = Mdl; + NET_BUFFER_DATA_LENGTH(Nb) = PacketSize; + NET_BUFFER_DATA_OFFSET(Nb) = NET_BUFFER_CURRENT_MDL_OFFSET(Nb) = 0; Nbl->SourceHandle = Ctx->MiniportAdapterHandle; NdisSetNblFlag(Nbl, NblFlags); NET_BUFFER_LIST_INFO(Nbl, NetBufferListFrameType) = (PVOID)NblProto;