Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usability improvements #119

Open
Unkorunk opened this issue Mar 3, 2024 · 10 comments
Open

Usability improvements #119

Unkorunk opened this issue Mar 3, 2024 · 10 comments

Comments

@Unkorunk
Copy link

Unkorunk commented Mar 3, 2024

  1. Define the CTL_CODE macro and the IoGetCurrentIrpStackLocation function.

  2. Move constants and macros (such as FILE_DEVICE_UNKNOWN, METHOD_BUFFERED, and others) from this crate and windows-rs to a shared crate.
    Motivation: This enables the definition of IOCTL codes in a shared crate that can be linked to both the driver and application sides.

  3. Change the types of some constants, like IRP_MJ_* to usize, and IO_NO_INCREMENT to CCHAR. It would also be beneficial to define TRUE and FALSE as pub const TRUE: BOOLEAN = 1; and pub const FALSE: BOOLEAN = 0; respectively, or even better, eliminate all BOOLEAN types from the API and replace them with the plain Rust bool, although I understand that this might be nearly impossible.
    Motivation: To eliminate unnecessary type casting where it logically should not exist.

  4. Do something with __bindgen_anon_*.
    Motivation: The statement (*irp).IoStatus.__bindgen_anon_1.Status = status; looks very awkward, and it's also challenging to locate the needed field in more complex cases like (*irp).Tail.Overlay.__bindgen_anon_2.__bindgen_anon_1.CurrentStackLocation.

  5. In general, it would be beneficial if constants in constants.rs had types from types.rs.

@0xflux
Copy link

0xflux commented Oct 13, 2024

Just checking - Is there no IoGetCurrentIrpStackLocation currently? Just checking as I'm looking for it in the docs and the repo but cant find it. If not - I may take a stab at implementing it myself as I'd love to contribute to this project! I also have a macro for CTL_CODE that I could contribute 🥳

@0xflux
Copy link

0xflux commented Oct 13, 2024

I've had a stab at binding this - I couldn't get it to bind via FFI for some reason, so I went into the MSDN and had a look at the C implementation in the header file (wdm.h) and I have come up with:

(note, this is also currently untested as of the time of writing)

pub unsafe fn IoGetCurrentIrpStackLocation(irp: PIRP) -> PIO_STACK_LOCATION {
    assert!((*irp).CurrentLocation <= (*irp).StackCount + 1); // todo maybe do error handling instead of an assert?
    (*irp).Tail.Overlay.__bindgen_anon_2.__bindgen_anon_1.CurrentStackLocation
}

I'm not sure if this is suitable for this project as it looks like mostly FFI bindings? If this is suitable for this project I would love to contribute it as my first addition, or to be advised on how it could be implemented using FFI; This function doesn't appear in the exports table of ntoskrnl.lib, so I assume that this project is looking (over time) to add implementations in Rust style for the wdk C header files?

If this is something i could contribute, I'll fix up any safety of the above snippet (its just a POC thing), handle errors appropriately, and hopefully get my first pull request into this awesome project 🥳

@Unkorunk
Copy link
Author

Unkorunk commented Oct 13, 2024

I think this is suitable for this project, since there are already some wrappers around low-level unsafe primitives. For example: https://github.com/microsoft/windows-drivers-rs/blob/main/crates/wdk/src/wdf/spinlock.rs. According to the README wdk crate should be suitable for safe primitives / utilities. But that's just my opinion; I think it's worth pinging one of the owners of this repository.

@Unkorunk
Copy link
Author

Unkorunk commented Oct 13, 2024

As for asserts, to be honest, I'm not sure if assert! and RtlAssert are interchangeable. What's the difference between them, and which one should be used when writing such utilities?

As for whether to use assert! or something like Result, I think it depends on the fact that this particular assert was written to track situations where the method was called at the wrong time (i.e., a bug in the driver), and therefore an assert! should be used here instead of error handling. That is, a bug in the driver is more of an unrecoverable error (https://doc.rust-lang.org/book/ch09-00-error-handling.html).

@0xflux
Copy link

0xflux commented Oct 14, 2024

@wmmc88 Hey! I'm not sure if this is something you are able to comment on? I'd love to be able to contribute something here :)

@wmmc88
Copy link
Collaborator

wmmc88 commented Oct 15, 2024

As for asserts, to be honest, I'm not sure if assert! and RtlAssert are interchangeable. What's the difference between them, and which one should be used when writing such utilities?

Per https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/_kernel/, RtlAssert should not be used within a driver.

Is there no IoGetCurrentIrpStackLocation currently?

Bindgen doesn't generate a binding to it because its an inline function. There is no symbol to link a bindgen-generated function to.

I'm not sure if this is suitable for this project as it looks like mostly FFI bindings? If this is suitable for this project I would love to contribute it as my first addition, or to be advised on how it could be implemented using FFI; This function doesn't appear in the exports table of ntoskrnl.lib, so I assume that this project is looking (over time) to add implementations in Rust style for the wdk C header files?

I think this is suitable for this project, since there are already some wrappers around low-level unsafe primitives. For example: https://github.com/microsoft/windows-drivers-rs/blob/main/crates/wdk/src/wdf/spinlock.rs. According to the README wdk crate should be suitable for safe primitives / utilities. But that's just my opinion; I think it's worth pinging one of the owners of this repository.

Since this should be a 1:1 port of what's in the header, this belongs in the wdk-sys crate. For example, #183 added the NTSTATUS handling apis that weren't generated because they're inlined in the headers. wdk crate is supposed to be reserved for more idiomatic Rust safe apis.. but that work is still ongoing internally and heavily skewed towards WDF.

IoGetCurrentIrpStackLocation afaiu is solely for the more legacy-style WDM drivers. I would strongly suggest you to look towards using WDF instead: https://learn.microsoft.com/en-us/windows-hardware/drivers/wdf/wdf-porting-guide 

@0xflux
Copy link

0xflux commented Oct 15, 2024

Thanks for the comment, so you dont want a contribution for IoGetCurrentIrpStackLocation as it relates to WDM? Are you only porting WDF APIs?

@Unkorunk
Copy link
Author

RtlAssert should not be used within a driver.

But it looks like IoGetCurrentIrpStackLocation uses RtlAssert under the hood.

@0xflux
Copy link

0xflux commented Oct 18, 2024

@wmmc88 If IoGetCurrentIrpStackLocation is for legacy drivers, it's given clearly in examples from Windows Kernel Programming 2nd Edition by Pavel Yosifovich, I'm guessing given that it is still to be used as part of IOCTL functions? If so, I would still like to contribute it ^^, as it seems relevant and from what I know, Pavel is considered a leader in terms of driver development books.

Edit: Looking back, he does indicate that it is using a WDM project.

@0xflux
Copy link

0xflux commented Dec 6, 2024

@Unkorunk Just FYI I have submitted a PR for the IoGetCurrentIrpStackLocation function! #251

It's my first PR into this project, it's a small addition but I'm excited for it! Hopefully it passes :) 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants