Problem with fragmented ICMPv6 in release_21H2

lstipakov commented 2 years ago

Hi guys,

I am a developer of ovpn-dco-win driver, the new driver which OpenVPN will use on Windows starting from release 2.6. This driver uses NetAdapter framework. While working on test automation, I noticed that ping tests, which use IPv6 and send large ICMP packets (causing IPv6 fragmentation), fail - we never got ICMP response.

I did packet capture with Wireshark on my driver and couldn't find anything wrong:

1   0.000000        fd00:abcd:194:1::1018   fd00:abcd:194:1::1      IPv6    1496    IPv6 fragment (off=0 more=y ident=0xa15b8e69 nxt=58)
2   0.000000        fd00:abcd:194:1::1018   fd00:abcd:194:1::1      ICMPv6  608     Echo (ping) request id=0x0001, seq=30, hop limit=128 (reply in 4)
3   0.315400        fd00:abcd:194:1::1      fd00:abcd:194:1::1018   IPv6    1496    IPv6 fragment (off=0 more=y ident=0x99a879b6 nxt=58)
4   0.473348        fd00:abcd:194:1::1      fd00:abcd:194:1::1018   ICMPv6  608     Echo (ping) reply id=0x0001, seq=30, hop limit=64 (request in 2)

Next I tried pktmon and found something interesting:

11:06:11.828637100 Drop: PktGroupId 562949953421349, PktNumber 1, Appearance 1, Direction Rx , Type IP , Component 1, Filter 0, DropReason Unsupported EtherType , DropLocation 0xE0001007, OriginalSize 1496, LoggedSize 128
    Drop: ip: fd00:abcd:194::1 > fd00:abcd:194:1::1018: frag (0|1448) ICMP6, echo reply, seq 195, length 1448
11:06:11.984539400 Drop: PktGroupId 1125899906843420, PktNumber 1, Appearance 1, Direction Rx , Type IP , Component 1, Filter 0, DropReason Unsupported EtherType , DropLocation 0xE0001007, OriginalSize 608, LoggedSize 128
    Drop: ip: fd00:abcd:194::1 > fd00:abcd:194:1::1018: frag (1448|560)`

As you see, packet was dropped by network stack with the reason Unsupported EtherType. Since my driver works on Layer 3 and I don't deal with ethernet headers, I looked into NetAdapter source code and traced packet indication with windbg. I have now reasons to believe that NetAdapter release_21H2 doesn't work with fragmented ICMPv6 packets. The previous version (for example release_2004) works just fine.

The bug seems to be in ParseIPv6ExtensionLayout() function, see https://github.com/microsoft/Network-Adapter-Class-Extension/blob/release_21H2/netcx/translator/framelayout/layer3layout.cpp#L184

Have a look at this code:

switch (extension->Header.NextHeader)
{
case NextHeader::TCP:
    Layout.Layer4Type = NetPacketLayer4TypeTcp;
    break;

case NextHeader::UDP:
    Layout.Layer4Type = NetPacketLayer4TypeUdp;
    break;

case NextHeader::NoNextHeader:
    nextHeader = false;
    break;
}

With fragmented ICMPv6, extension->Header.NextHeader value is 0x3a (IPv6-ICMP). This is not handled by the switch above, so the parser tries to parse the next extension (line 165), starting to look into the payload. In the end parsing fails and we exit on the line 172. As a result of that, Layout.Layer3Type is not set to NetPacketLayer3TypeIPv6WithExtensions and stays 0.

Further down the stack in RxMetadataTranslator::TranslatePacketLayout() function we translate packet layout to NDIS. If packet.Layout.Layer3Type is 0, we try to read it from ethernet header (ParseLayer3Type() on line 182), but this also returns zero since Layout.Layer2Type is set to NetPacketLayer2TypeNull and not to NetPacketLayer2TypeEthernet. Next we indicate the incorrect layer3type to NDIS:

    // NetBufferListFrameType expects layer 3 type in network byte order
    NetBufferList.NetBufferListInfo[NetBufferListFrameType] = reinterpret_cast<void *>(layer3Type);

NetBufferListFrameType "Identifies a USHORT value that is the frame type of the received Ethernet packets". Since it is set to incorrect value, network stack drops the packet with "Unsupported EtherType" error, as I've seen with pktmon.

I believe that ParseIPv6ExtensionLayout() should handle ICMPv6 (and maybe other protocols?), break from the extensions parsing loop and assign Layer3Type to NetPacketLayer3TypeIPv6WithExtensions. In this case layer3type will be correctly indicated to NDIS by RxMetadataTranslator::TranslatePacketLayout():

case NetPacketLayer3TypeIPv6UnspecifiedExtensions:
case NetPacketLayer3TypeIPv6WithExtensions:
case NetPacketLayer3TypeIPv6NoExtensions:
    layer3Type = RtlUshortByteSwap(ETHERNET_TYPE_IPV6);
    NdisSetNblFlag(&NetBufferList, NDIS_NBL_FLAGS_IS_IPV6);
    break;

Please let me know if my analysis makes sense and if we may expect a fix.

tylerretzlaff commented 1 year ago

a fix for the reported problems should be available in windows insider builds with build number >= 25876. if you are able to test and verify that the build resolves the issue it would be great, otherwise thank you for reporting with exceptional detail it was very helpful.

summary of fixes:

presence of ipv6 fragment extensions should no longer discard or downgrade more specific driver provided metadata.
use of layer2 type null frames should not result in discard of driver provided metadata.

a slightly more detailed list of changes made is below:

collapse switch case for NoNextHeader into icmp cases since the switch case actions are common (i.e. we terminate parsing) but don't record layer4 type or length metadata. note: this is not a bug fix just a code redundancy removal/simplification.
only explicitly parse extension headers for known extension values instead of trying to parse for arbitrary unknown nextHeader values.
define Protocol::IPv6Fragment and corresponding NextHeader::IPv6FragmentHeader enumerations and use the enumerations to:
- explicitly terminate next header parsing when Protocol::IPv6Fragment is observed, during header or extension header parsing.
- explicitly invoke extension parsing when NextHeader::IPv6FragmentHeader is observed.
remove explicit failure of IPv6 layout parsing when there is an inability to parse extensions. instead succeed but leave layer4 metadata in the default state since we can only determine it by parsing all extensions.
add an explicit test to check if the driver specified NetPacketLayer2TypeNull and if so avoid parsing layout for layer2
tweak the sequence of conditional checks in ParseLayer2LayoutIfNeeded to be more linear and straight forward eliminating the need for unnecessary compound conditionals.
remove two redundant expressions from conditional checks in ParseLayer2LayoutIfNeeded

lstipakov commented 1 year ago

Hi @tylerretzlaff,

Will this fix be available for Windows 10, or is it only for Windows 11?

lstipakov commented 1 year ago

I use Canary channel and at 25381, I guess it might take a while for 25876 to arrive?

lstipakov commented 1 year ago

I can confirm that the problem is fixed in 23H2 (25905.1000).

microsoft / Network-Adapter-Class-Extension

Problem with fragmented ICMPv6 in release_21H2 #12