Make kernel reader minimal and read-only#1
Conversation
📝 WalkthroughWalkthroughThis pull request transforms a multi-feature kernel-memory-reader tool into a minimal read-only utility. The shared IOCTL protocol is simplified to a single request struct, the kernel driver is pared down to handle only buffered memory reads via ChangesKernel-Reader simplification to single read-only IOCTL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 525cc46725
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #define IOCTL_READ_PROCESS_MEMORY \ | ||
| CTL_CODE(FILE_DEVICE_KERNEL_READER, 0x800, METHOD_BUFFERED, FILE_READ_ACCESS) |
There was a problem hiding this comment.
When the existing Rust controller is used, it still imports IOCTL_DISPATCH from rust/shared/src/lib.rs and sends the old discriminated request structs from rust/controller/src/driver.rs, but this change makes the C driver accept only the new IOCTL_READ_PROCESS_MEMORY value and the smaller READ_MEMORY_REQUEST layout. That means the documented Rust controller now gets rejected before dispatch, or would have its payload misinterpreted if pointed at the new IOCTL; please update/remove the Rust shared/controller code or keep a compatibility path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
driver/driver.c (1)
148-154: 💤 Low valueUnsupported IRPs should fail rather than succeed silently.
The loop sets all major functions to
DispatchCreateClose, which returnsSTATUS_SUCCESS. This means unsupported operations likeReadFile/WriteFilesucceed with 0 bytes instead of failing. Consider returningSTATUS_INVALID_DEVICE_REQUESTfor unsupported IRPs.Suggested approach
+static NTSTATUS DispatchUnsupported(PDEVICE_OBJECT DeviceObject, PIRP Irp) { + UNREFERENCED_PARAMETER(DeviceObject); + return CompleteIrp(Irp, STATUS_INVALID_DEVICE_REQUEST, 0); +} + // In DriverEntry: for (ULONG i = 0; i <= IRP_MJ_MAXIMUM_FUNCTION; ++i) { - DriverObject->MajorFunction[i] = DispatchCreateClose; + DriverObject->MajorFunction[i] = DispatchUnsupported; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@driver/driver.c` around lines 148 - 154, The current loop assigns DriverObject->MajorFunction[*] = DispatchCreateClose which causes unsupported IRPs to return STATUS_SUCCESS; change the default assignment to a new handler (e.g., DispatchUnsupported) that returns STATUS_INVALID_DEVICE_REQUEST, then explicitly set DriverObject->MajorFunction[IRP_MJ_CREATE] = DispatchCreateClose, [IRP_MJ_CLOSE] = DispatchCreateClose and [IRP_MJ_DEVICE_CONTROL] = DispatchDeviceControl as you already do; add a simple DispatchUnsupported(IRP, ...) implementation that completes the IRP with IoCompleteRequest and returns STATUS_INVALID_DEVICE_REQUEST so unsupported Read/Write/etc. fail properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@driver/driver.c`:
- Around line 148-154: The current loop assigns DriverObject->MajorFunction[*] =
DispatchCreateClose which causes unsupported IRPs to return STATUS_SUCCESS;
change the default assignment to a new handler (e.g., DispatchUnsupported) that
returns STATUS_INVALID_DEVICE_REQUEST, then explicitly set
DriverObject->MajorFunction[IRP_MJ_CREATE] = DispatchCreateClose, [IRP_MJ_CLOSE]
= DispatchCreateClose and [IRP_MJ_DEVICE_CONTROL] = DispatchDeviceControl as you
already do; add a simple DispatchUnsupported(IRP, ...) implementation that
completes the IRP with IoCompleteRequest and returns
STATUS_INVALID_DEVICE_REQUEST so unsupported Read/Write/etc. fail properly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95731d6c-7600-4586-a6b9-240ac6751948
📒 Files selected for processing (4)
README.mdclient/client.cppdriver/driver.cshared/ioctl_shared.h
1 participant
Motivation
CTL_CODEand a simple request struct.METHOD_BUFFERED,MmCopyVirtualMemory, IRQL checks, input/output length validation, and strict lifetime discipline forPEPROCESSreferences.Description
client/client.cppthat acceptsclient.exe <pid> <hex-address> <size>, opens\\.\KernelReader, issuesDeviceIoControlwithIOCTL_READ_PROCESS_MEMORY, and hex-dumps the returned bytes.driver/driver.cto a focused WDM implementation that creates the device and DOS symbolic link, handlesIRP_MJ_CREATE/IRP_MJ_CLOSE, and services a single buffered IOCTL read path which validates inputs, requiresPASSIVE_LEVEL, looks up the target process withPsLookupProcessByProcessId, callsMmCopyVirtualMemory, returns the copied byte count, and always dereferences the process object.shared/ioctl_shared.hto defineIOCTL_READ_PROCESS_MEMORYviaCTL_CODE(FILE_DEVICE_KERNEL_READER, 0x800, METHOD_BUFFERED, FILE_READ_ACCESS)and the exactREAD_MEMORY_REQUESTlayout containingProcessId,TargetAddress, andSize.README.mdas an educational guide describing the architecture, the shared wire format, build/load instructions, stability checklist, and how this maps to a Valthrun-style Rust port.Testing
git diff --check(no whitespace or diff errors) and committed the changes; this check succeeded.cl/msbuild) in the container and found the Visual Studio/WDK toolchain unavailable, so no driver or client builds were executed in CI here..sysbuild and kernel testing require a Windows VM with WDK and test-signing enabled).Codex Task
Summary by CodeRabbit
Refactor
Documentation