fix(windows-kext): fix potential BSODs and block verdict bypass in WFP callouts#2136
Merged
stenya merged 9 commits intodevelopmentfrom Mar 10, 2026
Merged
fix(windows-kext): fix potential BSODs and block verdict bypass in WFP callouts#2136stenya merged 9 commits intodevelopmentfrom
stenya merged 9 commits intodevelopmentfrom
Conversation
… params FwpsInjectTransportSendAsync1 dereferences FWPS_TRANSPORT_SEND_PARAMS1 (and remote_address within it) asynchronously after the callsite returns. The params were stack-allocated, so WFP may have accessed freed stack memory at DISPATCH_LEVEL, potentially causing PAGE_FAULT_IN_NONPAGED_AREA or DRIVER_IRQL_NOT_LESS_OR_EQUAL BSODs. Candidate fix: embed send_params in TransportPacketList, box the whole struct before calling the inject API, and populate send_params (remote_address points into boxed remote_ip) only after boxing so all pointers are into stable non-paged heap memory. Introduce free_transport_packet as the WFP completion callback that drops Box<TransportPacketList> once injection is complete. safing/portmaster-shadow#38
…llout returns FwpsInjectTransportSendAsync1 reads control_data (WSACMSGHDR) asynchronously after the ALE classify callout returns. The pointer was into WFP-managed metadata memory that WFP frees immediately when the callout returns, leaving a dangling pointer used during deferred injection. This could cause PAGE_FAULT_IN_NONPAGED_AREA or DRIVER_IRQL_NOT_LESS_OR_EQUAL BSODs. Candidate fix: change TransportPacketList.control_data from Option<NonNull<[u8]>> to Option<Box<[u8]>> and copy the bytes inside from_ale_callout while the WFP pointer is still valid. The owned copy lives on the heap as part of the boxed TransportPacketList context and is freed by free_transport_packet after WFP calls the completion callback. safing/portmaster-shadow#38
When completing a Reauthorization classify defer, reset_all_filters() would fail with STATUS_FWP_TXN_IN_PROGRESS if another WFP transaction was already running (e.g. from a concurrent ClearCache command). This caused the packet to be silently dropped instead of injected. This error is safe to ignore: the concurrent transaction will trigger WFP reauthorization for all connections anyway, and the verdict for the current connection is already written to the connection_cache before complete() is called, so the callout will apply the correct verdict when the injected packet passes through. All other errors from reset_all_filters() are still propagated.
… WFP filters Rename action_block() to action_block_hard(), which additionally calls clear_write_flag(). This prevents lower-weight filters in the same WFP sublayer from overwriting a block action set by Portmaster's callout. Updated call sites: - ALE layer: PermanentBlock, Undeterminable, Failed, and inbound Block - Packet layer: PermanentBlock
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR introduces thread-safe device access via AtomicPtr, refactors memory management in transport packet injection, changes block behavior to enforce immutable verdicts, updates API signatures with explicit lifetimes, adds documentation and tooling for debug driver builds, and removes a public module export. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses several root causes of BSODs in the Windows kernel extension and hardens the WFP callout logic.
Changes
BSOD Fixes
Heap-allocate WFP transport send params (
packet.rs)FWPS_TRANSPORT_SEND_PARAMS1andremote_ipare now stored inside a heap-allocatedBox<TransportPacketList>. Previously they lived on the stack and could be freed beforeFwpsInjectTransportSendAsync1finished reading them asynchronously.Copy WFP control data before callout returns (
packet.rs)The
control_datapointer passed by WFP is only valid for the duration of the classify callback. The bytes are now copied into an ownedBox<[u8]>instead of holding a raw pointer that would dangle after the callout returned.Replace global device pointer with
AtomicPtr(entry.rs)The
static mut DEVICEraw pointer is replaced withAtomicPtr<Device>with properAcquire/Releaseordering, preventing data races between concurrent callout threads and the load/unload path.WFP Verdict Handling
Prevent downstream filters from overriding block verdicts (
callout_data.rs,ale_callouts.rs,packet_callouts.rs)Replaced
action_block()withaction_block_hard()which additionally clears theFWPS_RIGHT_ACTION_WRITEflag, ensuring that lower-priority WFP filters in the chain cannot override a block decision.Ignore
STATUS_FWP_TXN_IN_PROGRESSinreset_all_filters(callout_data.rs)When another WFP transaction is already in progress,
reset_all_filtersreturns this status. It is now treated as a no-op: the concurrent transaction will trigger the same reauthorization, and the verdict is already in the connection cache.Code Quality
advance()to take&mut selfinstead of&selfspin_lockmoduleBUILD_DEBUG.mdandbuild_test.ps1for local test-signed driver buildsTesting
Tested locally with a test-signed release build. See
test/BUILD_DEBUG.mdfor setup instructions.Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores