Multiple bugs and critical errors in windows-kext driver - #2225
Multiple bugs and critical errors in windows-kext driver#2225GerardoKermitToy wants to merge 12 commits into
Conversation
Add protocol check to only process UDP (protocol 17) in stream callouts. Without this filter, all non-TCP IP traffic (ICMP, IGMP, IPsec, etc.) was incorrectly handled as UDP, polluting UDP statistics. For example, `ping 127.0.0.1` (ICMP) was being counted as UDP traffic.
Holding outbound UDP datagrams at the ALE layer corrupts the send status reported to applications using Registered I/O (RIOSendEx). Both pend_operation and absorb cause datagrams to complete with WSAEINVAL even when successfully delivered, causing applications to retry and duplicate packets. Solution: Move outbound UDP verdict decisions from ALE layer to IP packet layer. The packet layer sits below the socket, so send operations complete successfully before classification. Register connections at ALE (where PID is available) but permit immediately and decide at packet layer. Inbound UDP unaffected - already classified at packet layer. Impact: Fixes WSAEINVAL errors for RIOSendEx UDP sends, eliminating packet duplication from application retries. Verified with 6 concurrent RIOSendEx calls all reporting success.
This patch addresses several critical parsing issues in the WFP driver related to variable-length IP headers: Key Fixes 1. IPv6 Extension Header Chain Resolution Implemented walk_ipv6_headers() to properly traverse extension header chains (Hop-by-Hop, Routing, Destination Options, Fragment) Fixed incorrect protocol identification when extension headers are present Previously, next_header was read directly from the base IPv6 header, causing protocol type misidentification (e.g., 60/43 instead of 17 for UDP) Added bounds checking with MAX_IPV6_EXT_HEADERS=8 to prevent infinite loops from malformed packets 2. IPv4 Options Handling Fixed transport header offset calculation to respect IHL (Internet Header Length) field Previously used fixed IPV4_HEADER_LEN (20 bytes), which caused port fields to be read from inside IP options when present Now reads up to IPV4_MAX_HEADER_LEN (60 bytes) and uses header_len() for correct offset 3. Inbound Packet Retreat Correction retreat_to_ip_header() now uses WFP's FWPS_METADATA_FIELD_IP_HEADER_SIZE instead of fixed constants Fixes buffer positioning for packets with IP options or extension headers Applies to both packet layer and ALE layer inbound processing 4. WFP Field Type Validation Added get_u32_or_zero() helper that checks ValueType before reading union members Fixes issue where InterfaceIndex and SubInterfaceIndex were read as garbage at ALE connect layer (fields are FWP_EMPTY at that point) Similar fix for FWP_DIRECTION field at datagram data layers (declared as FWP_UINT32, not FWP_UINT8) 5. Bounds Checking for Port Parsing Added length validation in get_ports() to prevent out-of-bounds access Prevents machine hang from panic when transport header is truncated or offset is incorrect Returns (0, 0) instead of panicking These issues caused: Connection keys with wrong protocol/ports being cached (cache poisoning) Verdicts applied to wrong connections IPv6 packets with extension headers being misidentified IPv4 packets with options having ports read from wrong offset Interface injection parameters containing garbage values All parsing now correctly handles variable-length headers according to RFC specifications.
This patch addresses two critical issues in UDP bandwidth statistics tracking at the Stream/Datagram layer: 1. Outbound packets overcounted by 8 bytes: The old code summed nbl.get_data_length() directly, which includes the UDP header (8 bytes) for outbound traffic but not for inbound. This caused outbound bandwidth to be inflated by the header size. 2. Re-injected packets double-counted: Outbound re-injected packets were counted twice—once on original indication and again when the driver's own injected copy passed through the layer.
This patch solves the following issue: 1. All fragmented incoming udp traffic was dropped
The following issue resolved: 1. Inbound TCP/UDP connections reported PID 0. 2. ICMP reported PID 0. 1. Loopback echo replies were reported with the wrong direction.
…t the ALE endpoint closure layers. Both handlers now require the full tuple — local address, local port, remote address, remote port — to be present and of the expected type before constructing the key. A closure indication without a remote peer is ignored, which is the correct outcome: there is no packet-layer connection keyed on it to end. No behaviour change for regular connected TCP/UDP closures, where all four fields are populated.
`ConnectionMap` groups connections by `(protocol, local port)` and every lookup scanned the whole vector for that port. The scan runs on the packet path, inside a `RwSpinLock`, at DISPATCH_LEVEL — so its cost is paid per packet, by every CPU that touches the same port. On a port carrying many connections at once (a busy listener, or an inbound flood aimed at one port) that is the dominant cost of the lookup, and it grows with the number of entries the port has accumulated: ended-but-not-yet-swept entries stay in the vector for up to a minute. Each per-port vector is now kept sorted by remote endpoint (`Connection::remote_key()` = `(remote_address, remote_port)`), so `read` / `get_mut` / `end` locate the entry with a binary search instead of a scan. `add` inserts at the right position rather than pushing.
Some TCP connections was reported with process ID 0.
Track TCP and UDP endpoint ownership from ALE resource assignment in a bounded, allocation-safe cache and resolve the local owner when packet-layer metadata has no process ID. Record TCP listeners at ALE_AUTH_LISTEN,refresh pre-existing listeners at ALE_AUTH_RECV_ACCEPT, and remove entries only when the releasing PID still owns the slot. Register the inspection callouts for IPv4 and IPv6 and keep existing nonzero connection PIDs protected from later zero or unrelated updates.
Register inspection callouts on ALE_FLOW_ESTABLISHED_V4/V6 layers to catch TCP connections that were created at the packet layer with PID=0 during the handshake race window. The flow established layer fires immediately after the three-way handshake completes, giving one final opportunity to resolve PID=0 entries before they become visible in logs or bandwidth statistics. Does not help pre-existing connections (handshake completed before driver load), but eliminates the most common source of PID=0 entries for connections established after the driver starts.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
|
Hello, Thank you for your efforts. This is a substantial PR that will require a detailed review, analysis, and testing on our side, especially since it involves kernel-level code. Unfortunately, I don't know when we will have enough time to properly review and test it, so I can't make any promises or provide an estimate at this point. By the way, it would be very helpful to have the following information included here:
Having this information would help us better understand the changes and make the review more efficient. |
Some bugs can be seen as follows:
The utilities I use in these tests can be found in my profile. |
|
I'll also add that I have a yearly paid subscription to your software and at least one my program(wireguard vpn) doesn't work correctly due to bugs in your driver. |
Fixes packet loss when WFP provides multiple NET_BUFFER structures in a single NET_BUFFER_LIST. Previously, the packet-layer clone path copied only first_net_buffer. The original NBL was then blocked and absorbed while only the first cloned packet was retained for verdict processing and reinjection. As a result, subsequent packets in the same NBL were silently dropped. This path was reproduced with normal Firefox TCP traffic, where WFP supplied two packets in one outbound NBL. This change: clones every NET_BUFFER into an independently owned NBL; keeps all cloned packets under the same pending verdict; recalculates checksums for every outbound clone; applies redirects to every packet in the batch; reinjects every clone after an accepting verdict; preserves the existing behavior of exposing the first packet as the representative payload to userspace. A batched NBL is now reinjected as multiple asynchronous WFP injection requests. Therefore, an immediate failure during a later injection can still result in partial delivery because earlier injections cannot be rolled back. The implementation also continues to apply one connection key and one verdict, derived from the first packet, to the entire batch.
|
Found new BUG: packet loss when WFP provides multiple NET_BUFFER structures in a single NET_BUFFER_LIST |
3 participants
I have already sent detailed information about many of these bugs to the technical support email.
This code was written entirely by Claude Opus 5.
I tested it with specialized diagnostic tools and found no serious problems.
But this does not mean that there are no bugs, there is room for improvements.