microsoft / windows-drivers-rs

Platform that enables Windows driver development in Rust. Developed by Surface.
Apache License 2.0
1.49k stars 65 forks source link

Usability improvements #119

Open Unkorunk opened 7 months ago

Unkorunk commented 7 months ago
  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 commented 4 days ago

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 commented 4 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 2 days ago

@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 commented 2 days ago

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 commented 2 days ago

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