Is this a bug in WDK's kbfiltr/moufiltr?

NTDEV
1

Hello,

I think the following code is not thread-safe:

NTSTATUS
KbFilter_InternIoCtl(
__in PDEVICE_OBJECT DeviceObject,
__in PIRP Irp
)
{
[…]

//
// Connect a keyboard class device driver to the port driver.
//
case IOCTL_INTERNAL_KEYBOARD_CONNECT:
//
// Only allow one connection.
//
if (devExt->UpperConnectData.ClassService != NULL) {
status = STATUS_SHARING_VIOLATION;
break;
}

[…]

//
// Copy the connection parameters to the device extension.
//
connectData = ((PCONNECT_DATA)
(irpStack->Parameters.DeviceIoControl.Type3InputBuffer));

devExt->UpperConnectData = *connectData;

[…]
}

I think “if (devExt->UpperConnectData.ClassService != NULL)” should be replaced with InterlockedCompareExchangePointer, but maybe it’s considered negligible because of likeliness.

2

Technically, you are correct, but practically it is of little concern. The only way this would cause a problem is if there was some filter driver in-between this filter and kbdclass which decided to send two IOCTL_INTERNAL_KEYBOARD_CONNECTs down the stack immediately (with different CONNECT_DATAs).

Not likely.

-Steve

3

This is not a problem at all. Kbdclass sends the connect ioctl and the contract in the stack is that it is only sent once.

d

-----Original Message-----
From: xxxxx@lists.osr.com [mailto:xxxxx@lists.osr.com] On Behalf Of xxxxx@gmail.com
Sent: Friday, December 21, 2007 11:04 AM
To: Windows System Software Devs Interest List
Subject: RE:[ntdev] Is this a bug in WDK’s kbfiltr/moufiltr?

Technically, you are correct, but practically it is of little concern. The only way this would cause a problem is if there was some filter driver in-between this filter and kbdclass which decided to send two IOCTL_INTERNAL_KEYBOARD_CONNECTs down the stack immediately (with different CONNECT_DATAs).

Not likely.

-Steve

NTDEV is sponsored by OSR

For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars

To unsubscribe, visit the List Server section of OSR Online at http://www.osronline.com/page.cfm?name=ListServer

4

Doron Holan wrote:

This is not a problem at all. Kbdclass sends the connect ioctl and the contract
in the stack is that it is only sent once.

So, since that’s the contract, you don’t have to check at all?

5

Yes, but the code is trying to be a bit defensive. Yes, it could be more defensive and use an interlocked operation, but that is a bit like using a shotgun to kill a fly in this case, esp since all other dereferences are not done with an interlocked operation

d

-----Original Message-----
From: xxxxx@lists.osr.com [mailto:xxxxx@lists.osr.com] On Behalf Of xxxxx@hushmail.com
Sent: Friday, December 21, 2007 1:09 PM
To: Windows System Software Devs Interest List
Subject: RE:[ntdev] Is this a bug in WDK’s kbfiltr/moufiltr?

Doron Holan wrote:

This is not a problem at all. Kbdclass sends the connect ioctl and the contract
in the stack is that it is only sent once.

So, since that’s the contract, you don’t have to check at all?

NTDEV is sponsored by OSR

For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars

To unsubscribe, visit the List Server section of OSR Online at http://www.osronline.com/page.cfm?name=ListServer

6

I see a lot of code like this in drivers written by people who aren’t
very comfortable with the debugger. The code doesn’t truly require
the check, since the contract makes sure it is valid. But the failure
mode is hard for some people to debug, so they write code that is
essentially an active comment, intended to make it clear that the
pointer should be valid.

Honestly, it seems like bloat to me. It should just be a comment, or
an assertion at best.

  • Jake Oshins

wrote in message news:xxxxx@ntdev…
> Doron Holan wrote:
>> This is not a problem at all. Kbdclass sends the connect ioctl and
>> the contract
>> in the stack is that it is only sent once.
>
> So, since that’s the contract, you don’t have to check at all?
>

7

Jake Oshins wrote:

Honestly, it seems like bloat to me. It should just be a comment, or
an assertion at best.

Would you also convert such lines to ASSERTs (for internal IOCTLs only):

if (irpStack->Parameters.DeviceIoControl.InputBufferLength <
sizeof(CONNECT_DATA)) {
//
// invalid buffer
//
status = STATUS_INVALID_PARAMETER;
break;
}

8

That really depends on whether I expected the contract in question to
change over time. If it was a very-long-standing IOCTL that couldn’t
possibly be changed, I probably would.

  • Jake

wrote in message news:xxxxx@ntdev…
> Jake Oshins wrote:
>> Honestly, it seems like bloat to me. It should just be a comment,
>> or
>> an assertion at best.
>
> Would you also convert such lines to ASSERTs (for internal IOCTLs
> only):
>
> if (irpStack->Parameters.DeviceIoControl.InputBufferLength <
> sizeof(CONNECT_DATA)) {
> //
> // invalid buffer
> //
> status = STATUS_INVALID_PARAMETER;
> break;
> }
>