Convert DeviceComm into an interface and add handle exchange#8367
Conversation
This commit replaces DeviceComm with the interface IDeviceComm and the concrete implementation type ConDrvDeviceComm. This work is done in preparation for different device backends. In addition to separating out ConDrv-specific behavior, I've introduced a "handle exchange" interface. HANDLE EXCHANGE --------------- There are points where we give ConDrv opaque handle identifiers to our input buffer, output buffer and process data. The exact content of the opaque identifier is meaningless to ConDrv: the driver's only interaction with these identifiers is to hold onto them and send back whichever are pertinent for each API call. Because of that, we used the raw register-width pointer value of the input buffer, output buffer or process data _as_ the opaque handle value. This works very well for ConDrv <-> conhost using the ConDrvDeviceComm. It works less well for something like the "logging" DeviceComm that will log packets to a file: those packets *cannot* contain pointer values (!) To address this, and to afford flexibility to DeviceComm implementers, I've introduced a two-member complement of handle management functions: * `ULONG_PTR PutHandle(void*)` registers an object with the DeviceComm and returns an opaque identifier. * `void* GetHandle(ULONG_PTR)` takes an opaque identifier and returns the original object. ConDrvDeviceComm implements PutHandle and GetHandle by casting the object pointer to the "opaque handle value", which maintains wire format compatibility[1] with revisions of conhost prior to this change. Simpler DeviceComm implementations that require handle tracking but cannot bear raw pointers can implement these functions by returning an autoincrementing ID (or similar) and registering the raw object pointer in a mapping. I've audited all existing handle exchanges with the driver and updated them to use Put/GetHandle. (I intended to add DestroyHandle, but we are not equipped for handle removal at the moment. ConsoleHandleData/ConsoleProcessHandle are destroyed during wait routine completion, on client disconnect, etc. This does mean that an id<->pointer mapping will grow without bound, but at a cost of ~8 bytes per entry and a short-lived console session I am not too concerned about the cost.) [1] Wire format compatibility is not required, and later we may want to switch ConDrvDeviceComm to `EncodePointer` and `DecodePointer` just to insulate us against a spurious ConDrv packet allowing for an arbitrary 4/8-byte read and subsequent litoff into space.
Is that because pointers in a log would thwart ASLR?
If conhost ever started tracking handles by default, that might be harmful for services that repeatedly start child processes. IIS and its w3wp.exe worker processes come to mind; w3wp.exe uses the console subsystem, but I don't know whether its parent process lets it inherit a console.
A related post about the security of those functions: The history of the EncodePointer function for obfuscating pointers | The Old New Thing |
Oh no, it’s because I want to capture binary ConDrv packet logs for replay and needed a stable identifier from session to session 😄 Having conhost keep track of all handles isn’t on my radar, but that is an excellent call-out. Thanks! Raymond’s blog is the only reason I even knew to suggest those functions! |
This was pulled out of branch |
LOL compatibility with giving out arbitrary heap addresses. |
|
@zadjii-msft (since you're assigned) i cleared the Draft status on this one |
|
Okay, I'll do a review pass on PRs probably tomorrow. |
zadjii-msft
left a comment
There was a problem hiding this comment.
I'm okay with this but pls add those doc comments
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
…ft#8367) This commit replaces DeviceComm with the interface IDeviceComm and the concrete implementation type ConDrvDeviceComm. This work is done in preparation for different device backends. In addition to separating out ConDrv-specific behavior, I've introduced a "handle exchange" interface. HANDLE EXCHANGE --------------- There are points where we give ConDrv opaque handle identifiers to our input buffer, output buffer and process data. The exact content of the opaque identifier is meaningless to ConDrv: the driver's only interaction with these identifiers is to hold onto them and send back whichever are pertinent for each API call. Because of that, we used the raw register-width pointer value of the input buffer, output buffer or process data _as_ the opaque handle value. This works very well for ConDrv <-> conhost using the ConDrvDeviceComm. It works less well for something like the "logging" DeviceComm that will log packets to a file: those packets *cannot* contain pointer values (!) To address this, and to afford flexibility to DeviceComm implementers, I've introduced a two-member complement of handle management functions: * `ULONG_PTR PutHandle(void*)` registers an object with the DeviceComm and returns an opaque identifier. * `void* GetHandle(ULONG_PTR)` takes an opaque identifier and returns the original object. ConDrvDeviceComm implements PutHandle and GetHandle by casting the object pointer to the "opaque handle value", which maintains wire format compatibility[1] with revisions of conhost prior to this change. Simpler DeviceComm implementations that require handle tracking but cannot bear raw pointers can implement these functions by returning an autoincrementing ID (or similar) and registering the raw object pointer in a mapping. I've audited all existing handle exchanges with the driver and updated them to use Put/GetHandle. (I intended to add DestroyHandle, but we are not equipped for handle removal at the moment. ConsoleHandleData/ConsoleProcessHandle are destroyed during wait routine completion, on client disconnect, etc. This does mean that an id<->pointer mapping will grow without bound, but at a cost of ~8 bytes per entry and a short-lived console session I am not too concerned about the cost.) [1] Wire format compatibility is not required, and later we may want to switch ConDrvDeviceComm to `EncodePointer` and `DecodePointer` just to insulate us against a spurious ConDrv packet allowing for an arbitrary 4/8-byte read and subsequent liftoff into space.
This commit replaces DeviceComm with the interface IDeviceComm and the
concrete implementation type ConDrvDeviceComm. This work is done in
preparation for different device backends.
In addition to separating out ConDrv-specific behavior, I've introduced
a "handle exchange" interface.
HANDLE EXCHANGE
There are points where we give ConDrv opaque handle identifiers to our
input buffer, output buffer and process data. The exact content of the
opaque identifier is meaningless to ConDrv: the driver's only
interaction with these identifiers is to hold onto them and send back
whichever are pertinent for each API call.
Because of that, we used the raw register-width pointer value of the
input buffer, output buffer or process data as the opaque handle
value.
This works very well for ConDrv <-> conhost using the ConDrvDeviceComm.
It works less well for something like the "logging" DeviceComm that will
log packets to a file: those packets cannot contain pointer values (!)
To address this, and to afford flexibility to DeviceComm implementers,
I've introduced a two-member complement of handle management functions:
ULONG_PTR PutHandle(void*)registers an object with the DeviceCommand returns an opaque identifier.
void* GetHandle(ULONG_PTR)takes an opaque identifier and returnsthe original object.
ConDrvDeviceComm implements PutHandle and GetHandle by casting the
object pointer to the "opaque handle value", which maintains wire format
compatibility[1] with revisions of conhost prior to this change.
Simpler DeviceComm implementations that require handle tracking but
cannot bear raw pointers can implement these functions by returning an
autoincrementing ID (or similar) and registering the raw object pointer
in a mapping.
I've audited all existing handle exchanges with the driver and updated
them to use Put/GetHandle.
(I intended to add DestroyHandle, but we are not equipped for handle
removal at the moment. ConsoleHandleData/ConsoleProcessHandle are
destroyed during wait routine completion, on client disconnect, etc.
This does mean that an id<->pointer mapping will grow without bound, but
at a cost of ~8 bytes per entry and a short-lived console session I am
not too concerned about the cost.)
[1] Wire format compatibility is not required, and later we may want to
switch ConDrvDeviceComm to
EncodePointerandDecodePointerjust toinsulate us against a spurious ConDrv packet allowing for an arbitrary
4/8-byte read and subsequent liftoff into space.