Validate Variable-Length Buffers- IOCTL (METHOD_BUFFERED)

I’m using a simple driver using WDM that does nothing but read/write to some IO location using IOCTL (METHOD_BUFFERED); the driver originally from some DDK source code using ‘portio’ sample.

One of the IOCTL’s input parameter has variable-length buffers and I’m following “Common Driver Reliability Issues”
http://msdn.microsoft.com/en-us/library/ms809962.aspx#drvrreliab_topic4

The driver works fine with variable buffer as long as the application DeviceIoControl passes correct buffer size; but when I test the driver with wrong buffer size I find that
“Failing to Validate Variable-Length Buffers” from the article does not work.

For example, I’m testing below by passing wrong buffer size.

DeviceIoControl(hDriver, // handle of communication port returned from CreateFile()
(DWORD) IOCTL_WRITE_IO_CFG, // operation to perform
&readIO, //address of the structure
sizeof(readIO)+reqSize, // wrong size of the structure
NULL,
0,
&dwBytesReturned,
NULL);

This is a very old issue, I believe I’m missing something but would like to get some tips or hints.

Thanks,
Hakim

How is it failing? How are you validating? The driver must validate that the input and output buffer lengths are the minimum values required , but for a buffered ioctl you cannot detect if the app specified a size that does not match the actual size of the buffer.

d

debt from my phone

-----Original Message-----
From: xxxxx@yahoo.ca
Sent: Tuesday, July 05, 2011 7:36 AM
To: Windows System Software Devs Interest List
Subject: [ntdev] Validate Variable-Length Buffers- IOCTL (METHOD_BUFFERED)

I’m using a simple driver using WDM that does nothing but read/write to some IO location using IOCTL (METHOD_BUFFERED); the driver originally from some DDK source code using ‘portio’ sample.

One of the IOCTL’s input parameter has variable-length buffers and I’m following “Common Driver Reliability Issues”
http://msdn.microsoft.com/en-us/library/ms809962.aspx#drvrreliab_topic4

The driver works fine with variable buffer as long as the application DeviceIoControl passes correct buffer size; but when I test the driver with wrong buffer size I find that
“Failing to Validate Variable-Length Buffers” from the article does not work.

For example, I’m testing below by passing wrong buffer size.

DeviceIoControl(hDriver, // handle of communication port returned from CreateFile()
(DWORD) IOCTL_WRITE_IO_CFG, // operation to perform
&readIO, //address of the structure
sizeof(readIO)+reqSize, // wrong size of the structure
NULL,
0,
&dwBytesReturned,
NULL);

This is a very old issue, I believe I’m missing something but would like to get some tips or hints.

Thanks,
Hakim


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

I’ve this
typedef struct _readIO{
ULONG Offset;
ULONG Size;
UCHAR *Data;
} readIO, *PReadIO;

if (InputBufferLength < sizeof(readIO)) {
IoCompleteRequest( Irp, STATUS_INVALID_PARAMETER );
return( STATUS_INVALID_PARAMETER );
}

then the following code
pReadIO = (PReadIO)Irp->AssociatedIrp.SystemBuffer;

if (FIELD_OFFSET(readIO, Data[0]) +
pReadIO->Size > InputBufferLength) {
IoCompleteRequest( Irp, STATUS_INVALID_PARAMETER );
return( STATUS_INVALID_PARAMETER );

The main point is as you mentioned
"but for a
buffered ioctl you cannot detect if the app specified a size that does not match
the actual size of the buffer.
"

How do I validate the variable size buffer ‘Data’? Changing the IOCTL method?
The buffer size is small and less than 100.

Thanks,
Hakim

>then the following code

pReadIO = (PReadIO)Irp->AssociatedIrp.SystemBuffer;

if (FIELD_OFFSET(readIO, Data[0]) +
pReadIO->Size > InputBufferLength) {
IoCompleteRequest( Irp, STATUS_INVALID_PARAMETER );
return( STATUS_INVALID_PARAMETER );

Lots of things going wrong here…

First of all, that’s the wrong way to check the length of variable length
structure. If Size is very large you will overflow and the test will pass.

Second, your structure has a pointer to the Data:

typedef struct _readIO{
ULONG Offset;
ULONG Size;
UCHAR *Data;
} readIO, *PReadIO;

Not a variable length tail containing the data:

typedef struct _readIO{
ULONG Offset;
ULONG Size;
UCHAR Data[1];
} readIO, *PReadIO;

So, your test wouldn’t be correct for the data structure as you have it
defined. In order to validate your structure you need to treat the Data and
Size fields as a METHOD_NEITHER buffer/length (i.e. ProbeForRead, build an
MDL, etc.).

Lastly, I’m not sure what you’re expecting to fail. In the OP you just
increased the InputBufferLength of the device control. As long as the I/O
manager can successfully copy sizeof(readIO)+foo from the input buffer
supplied things will, “just work.” Try running the application under
Application Verifier and see if you don’t get a failure.

However, before you go any further I *strongly* suggest throwing away these
IOCTL definitions and picking something simpler (e.g. a variable length
structure or switching to METHOD_DIRECT and having the input buffer be
control info and the output buffer be payload). Dealing with embedded data
pointers like this is very difficult. In fact, this is almost the most
complicated IOCTL definition that you could have come up with (the fact that
it’s METHOD_BUFFERED saves you from being the most complicated).

-scott


Scott Noone
Consulting Associate and Chief System Problem Analyst
OSR Open Systems Resources, Inc.
http://www.osronline.com

wrote in message news:xxxxx@ntdev…

I’ve this
typedef struct _readIO{
ULONG Offset;
ULONG Size;
UCHAR *Data;
} readIO, *PReadIO;

if (InputBufferLength < sizeof(readIO)) {
IoCompleteRequest( Irp, STATUS_INVALID_PARAMETER );
return( STATUS_INVALID_PARAMETER );
}

then the following code
pReadIO = (PReadIO)Irp->AssociatedIrp.SystemBuffer;

if (FIELD_OFFSET(readIO, Data[0]) +
pReadIO->Size > InputBufferLength) {
IoCompleteRequest( Irp, STATUS_INVALID_PARAMETER );
return( STATUS_INVALID_PARAMETER );

The main point is as you mentioned
"but for a
buffered ioctl you cannot detect if the app specified a size that does not
match
the actual size of the buffer.
"

How do I validate the variable size buffer ‘Data’? Changing the IOCTL
method?
The buffer size is small and less than 100.

Thanks,
Hakim

Thanks for your time to take a look in my code and offer good suggestions.

You have not stated what “validate” means.

For example, if you point to a block of memory and give a length larger than
the block (as your example suggests) then all the I/O Manager does is
validate that starting at that address, for that many length bytes, the
buffer is valid for the operation (that is, it is readable in the case of
buffered I/O). So unless you are in the unlikely situation where the buffer
is at the end of a page and the next page does not exist, pretty much any
pointer into the stack or heap where you point is considered “valid” by the
I/O Manager (I note that although you show a variable called “readIO”, you
have neglected to show its declaration, and neglected to show the context of
that declaration, making any meaningful analysis of the problem difficult).
If you declared
BYTE readIO[somesize];
Then the & is not needed; if you did
SomeStructureName readIO;
Then the & is required. But it is hard to tell, since the declaration is
omitted. Note that the I/O Manager has a very limited notion of “validate”,
so if you did
SomeStructureName readIo;
SomeCriticalData Here;
MoreCriticalData HereToo;
SomeBigBlockOfMemory Block;

And you gave a length > sizeof(readIO) as your example shows, then the I/O
Manager considers the computation of readIo+sizeof(readIo)+reqsize to result
in an address which is still valid, and all the bytes between readIO and the
computed address are valid, so the buffer is considered valid. For read, it
will happily overwrite all those variables which follow readIO; for write,
it will happily write out complete nonsense.

If, on the other hand, you LOOK at irpStack->Parameters.DeviceIoControl and
compare the length fields with a known constant that you expect, and if it
is not equal you complete the IRP with STATUS_INVALID_PARAMETER, then your
driver has rejected something the I/O Manager may have already accepted, and
that’s just fine. But then it wouldn’t be a “variable-length buffer”. The
assumption is that if you pass a valid address+length in, the driver is free
to scribble over or read that complete set of bytes, and what you would then
have is a programming error by the caller of the DeviceIoControl, which is
not your problem to solve in the driver.

The article you cite merely says that you must not access data beyond the
range provided to you by the I/O Manager in the IRP. This is a “contract”.
The I/O Manager promises that you have a valid address and range of bytes;
your obligation under the contract is to promise to not write outside that
range. But the I/O Manager does not trouble itself to discover that the
programmer has written something stupid, such as specifying a range that
will cause the programmer’s variables to be overwritten; for example, if I
do
BYTE * p = malloc(30);
ReadFile(h, p, 60, &bytesRead, NULL);

Then I have made a Stupid Programming Error; I allocated 30 bytes and told
the I/O Manager I wanted to read 40 bytes. The I/O Manager does not have
any knowledge of the fact that I have done something remarkably stupid; it
merely validates that all the bytes from p to p + 59 are valid bytes that
can be written to. That is all it is required to do. The fact that I am
about to clobber storage headers, or other blocks of heap memory, is of no
interest to the I/O Manager. Since it does not know how the user heap
works, it cannot tell that I have written a really stupid piece of code.

Note (a) you cannot do anything about this in your driver and (b) you don’t
need to. The application has been coded incorrectly.

If, on the other hand, a DeviceIoControl is expected to fill a fixed-size
structure, and the size you pass in to that operation is *not* that expected
size, then you *can* and *should* detect this as a Stupid Programming Error
and, as I indicated, complete the IRP with a suitable error status. But
since you have indicated your buffer is of variable length, the onus of
specifying a meaningful and correct length rests on the application
programmer. All you have to do is meet your contractural obligations of not
writing outside the buffer you have been given.
joe

-----Original Message-----
From: xxxxx@lists.osr.com
[mailto:xxxxx@lists.osr.com] On Behalf Of xxxxx@yahoo.ca
Sent: Tuesday, July 05, 2011 10:37 AM
To: Windows System Software Devs Interest List
Subject: [ntdev] Validate Variable-Length Buffers- IOCTL (METHOD_BUFFERED)

I’m using a simple driver using WDM that does nothing but read/write to some
IO location using IOCTL (METHOD_BUFFERED); the driver originally from some
DDK source code using ‘portio’ sample.

One of the IOCTL’s input parameter has variable-length buffers and I’m
following “Common Driver Reliability Issues”
http://msdn.microsoft.com/en-us/library/ms809962.aspx#drvrreliab_topic4

The driver works fine with variable buffer as long as the application
DeviceIoControl passes correct buffer size; but when I test the driver with
wrong buffer size I find that “Failing to Validate Variable-Length Buffers”
from the article does not work.

For example, I’m testing below by passing wrong buffer size.

DeviceIoControl(hDriver, // handle of communication port returned from
CreateFile()
(DWORD) IOCTL_WRITE_IO_CFG, // operation to perform
&readIO, //address of the structure
sizeof(readIO)+reqSize, // wrong size of the structure
NULL,
0,
&dwBytesReturned,
NULL);

This is a very old issue, I believe I’m missing something but would like to
get some tips or hints.

Thanks,
Hakim


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


This message has been scanned for viruses and dangerous content by
MailScanner, and is believed to be clean.