Feature/kext default action drop #1747
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing packet management and control flow within the Windows kernel extension. Key updates include the introduction of default packet blocking behavior in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
windows_kext/wdk/src/filter_engine/classify.rs (1)
83-86: LGTM! Consider enhancing the documentation.The implementation correctly clears the absorb flag using bitwise operations. The method complements the existing
set_absorbfunctionality, providing a clean way to manage the absorb flag state.Consider expanding the documentation to match the detail level of other methods, explaining when this should be called:
- // Removes the absorb flag. + // Removes the absorb flag. This should be called when packet reinjection is no longer needed + // or to ensure the packet continues through the network stack.windows_kext/wdk/src/filter_engine/callout_data.rs (1)
Line range hint
188-193: Consider adding documentation for the block_and_absorb methodThe implementation looks good and provides explicit control over the absorb flag. Consider adding documentation to explain:
- The purpose of combining block and absorb actions
- When this method should be used vs. regular block
- The significance of the operation order (block then absorb)
+ /// Blocks the current operation and sets the absorb flag. + /// This combination is typically used when you want to both block the operation + /// and prevent further processing of the packet in the filter pipeline. pub fn block_and_absorb(&mut self) { unsafe { (*self.classify_out).action_block();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
windows_kext/driver/src/ale_callouts.rs(1 hunks)windows_kext/driver/src/packet_callouts.rs(2 hunks)windows_kext/wdk/src/filter_engine/callout_data.rs(1 hunks)windows_kext/wdk/src/filter_engine/classify.rs(1 hunks)windows_kext/wdk/src/filter_engine/ffi.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- windows_kext/wdk/src/filter_engine/ffi.rs
🔇 Additional comments (4)
windows_kext/wdk/src/filter_engine/callout_data.rs (1)
164-164: Verify the impact of always clearing the absorb flag
The implementation consistently clears the absorb flag after each action (permit, continue, block, none). While the pattern is implemented correctly, please verify that:
- Clearing the absorb flag after every action aligns with the Windows Filtering Platform (WFP) expectations
- There are no edge cases where we might want to preserve the absorb flag state
Also applies to: 171-171, 178-178, 185-185
windows_kext/driver/src/packet_callouts.rs (2)
113-114: Ensure legitimate packets are explicitly permitted after default blocking
With the addition of data.block_and_absorb(); at the start of the ip_packet_layer function, all packets are now blocked by default. Please verify that the subsequent logic correctly identifies and permits legitimate packets by calling data.action_permit(); to avoid unintended packet drops.
153-153: Confirm necessity of changing log level to err!
The log message has been changed from dbg! to err! in crate::err!("failed to get key from nbl: {}", err);. Confirm that this condition is severe enough to warrant error-level logging, or if it should remain at the debug level to prevent excessive logging during normal operation.
windows_kext/driver/src/ale_callouts.rs (1)
108-109: Ensure all code paths override the default block-and-absorb action
By adding data.block_and_absorb() at the beginning of ale_layer_auth, the default behavior is now to block and absorb packets unless explicitly overridden. Please verify that all code paths appropriately override this default action with data.action_permit() or data.action_block() where necessary, to prevent unintended packet drops.
* [windows_kext] Make default action to drop * [windows_kext] Minor improvments
Summary by CodeRabbit
New Features
Bug Fixes
Documentation