Bug in SwapBuffer WDK sample?

Hi all!

Recently I found suspicious code in SwapBuffer WDK sample.
I am not 100% sure, but it looks like RtlCopyMemory can touch memory outside original buffer and thus case access violation.
Let's take a look at the SwapPreWriteBuffers function:

// *1*
ULONG writeLen = iopb->Parameters.Write.Length;

if (FlagOn(IRP_NOCACHE,iopb->IrpFlags)) {
    writeLen = (ULONG)ROUND_TO_SIZE(writeLen,volCtx->SectorSize); // *2*
}

// *3*
RtlCopyMemory( newBuf, origBuf, writeLen );

For non-cached I/O, if 'writeLen' in '1' is not sector-aligned (for example, 400 instead of 512), then in '3' we copy more bytes from 'origBuf' than it actually has.

According to my knowledge, only kernel-mode callers can ignore alignment requirements for non-cached I/O, so importance of this issue is low.
But, still, may be it need to be fixed like this:
RtlCopyMemory( newBuf, origBuf, iopb->Parameters.Write.Length );
?

Or, may be, I don't fully understand why we need ROUND_TO_SIZE in both read and write callbacks...
Because in any case, an operation with incorrect parameters will be rejected either by I/O manager or file system,
or it will be 'fixed' through mechanism like 'FatNonCachedNonAlignedRead' (fastfat) and similar.

1 Like

Yeah, that code doesn't seem right, particularly in the read path...

Non-sector aligned, non-cached I/Os are definitely some weird, rare edge case. I can't think of a specific case that causes these at the moment and I tried a few things that I thought might but didn't have any luck making them happen.

The file systems DO handle them though and are pretty consistent...In the read path they take special care to deal with not overrunning the destination buffer (as exampled by FatNonCachedNonAlignedRead...NTFS also has a NtfsNonCachedNonAlignedIo that's called in the read path). However, the write path mostly just rounds up and doesn't care about overrunning the source.

Going back to SwapBuffers, the write path doesn't seem any worse than what you normally get. For read though copying more data back to the requestor definitely seems like a bug. This case is so weird though that I'm not sure anyone will ever notice...

But, you'd be able to pretty quickly prove the theory by writing a driver that sends unaligned non-cached reads. Seems like without SwapBuffers things would "just work" but with SwapBuffers you'd get a corruption at the end of your read buffer

Unaligned non-cached IO. Maybe there is there a bug in this code, but probably any caller that could bypass the checks to trigger it, ought to know better and not do that.

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.